Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a very basic script. If the screen width is larger than say a tablet, and a portfolio link is hovered over, it will fade out the original text and replace it with the text held in the 'data-portfolio' attribute. I'm fairly new to jQuery so any improvements will be welcomed.

You can see it working here.

(function () {

width = $(window).width();

if(width >= 620) {
    $('.portfolio-piece__link').hover(function(){

    var $this = $(this);
        prev = $this.text();
        data = $this.attr('data-portfolio');



        $this.fadeOut(300, function(){
            $this.text(data).fadeIn(300);
        });     

    }, function(){  

    var $this = $(this);    
        $this.fadeOut(300, function(){
            $this.text(prev).fadeIn(300);
        });     
    });
}

})();
share|improve this question
    
By the looks of it you have a bunch of implicit global variables, width, prev and data. Are those really meant to be global? Are we missing some code? –  elclanrs Mar 11 at 10:06
    
You are not missing code so I've clearly declared them incorrectly ha! –  jshjohnson Mar 11 at 11:40
add comment

2 Answers 2

up vote 7 down vote accepted

First off, you should clean up the indention. Currently the wrong indention makes the code hard to read.

The next problem is that your variables width, prev and data are lacking the var keyword making them global und thus making the surrounding IIFE pointless. Unfortunatly you are actually reading the prev variable in the second hover function globally. This will go especially wrong if you happen to have multiple elements with the class portfolio-piece__link one one page. You may want to move the definition of those variables outside of your hover functions and declare them locally inside the IIFE. That way it also has the advantage that you don't need to create them on each hover.

Then what you need to consider is your code will not react to window resizes. What happens if the user originally opens your page with a small window size (< 640) and then resizes the window. BTW, why do you want this effect only on larger window sizes in the first place?

Finally you may want to consider writing this to be generic and not hard code it to only work with that specific class and property name.

EDIT: Moving the variables outside the hover functions is of course not sensible, if you have multiple elements. I wrote that before I realized that. Instead you should store the old text, for example using jQuery's data method. Example:

function replaceText($element, text) {
   element.fadeOut(300, function(){
        $this.text(text).fadeIn(300);
    });     
}

$('.portfolio-piece__link').each(function() {
  // Store original text
  var $this = $(this);
  $this.data('original-text', $this.text());
}).hover(function(){
    var $this = $(this);
    var data = $this.attr('data-portfolio');

    replaceText($this, data);
}, function(){  
    var $this = $(this);    
    var text = $this.data('original-text');

    replaceText($this, text);
});

BTW, I forgot one thing originally: You have duplicate code for replacing the text. You should replace it with a method as I did above.

share|improve this answer
    
Thanks for giving such a detailed reply! Are you referring to the indention of variables or just the code in general - doesn't seem too off to me? Also, how would I go about declaring the "prev" variable for example, outside the hover fucntion? Surely if I put it outside, it wouldn't know what $this is and therefore not grab the previous text? I decided to apply the effect on larger screens purely for usability - on touch devices it just doesn't work as well. Thanks –  jshjohnson Mar 11 at 11:39
    
Most of the code is indented incorrectly - noticeable because the last line })(); isn't formatted properly. For the prev variable see my edit. However you have an error in reasoning: A small screen does not mean it's a phone/tablet. Not all vehicles with four wheel are cars. Not all cars have four wheels. Instead of checking the screen size, you should be checking if the browser is capable of hover events. Unfortunately I can't recall how you do that properly. Let me research it and I'll come back if I find something suitable. –  RoToRa Mar 11 at 11:57
add comment

From a once over:

  • This:

    var $this = $(this);
        prev = $this.text();
        data = $this.attr('data-portfolio');
    

    should have been this:

    var $this = $(this),      //<- Comma, not semicolon
        prev = $this.text(),  //<- Comma, not semicolon
        data = $this.attr('data-portfolio');
    
  • Be consistent with single comma separated var statements:

    }, function(){  
        var $this = $(this),  //<- Comma here
            text = $this.data('original-text');
    
        replaceText($this, text);
    });
    
  • As @elclanrs mentioned, prev is global, the only reason why your code works by the way..

If you think about it, both handlers do pretty much the same thing, so I would counter propose this:

function portfolioHoverHandler( $element ){
    var newText = $element.attr('data-portfolio');
    $element.attr('data-portfolio', $element.text());      
    element.fadeOut(300, function(){
        $this.text(newText).fadeIn(300);
    });    
}

$('.portfolio-piece__link').hover(portfolioHoverHandler, portfolioHoverHandler);

I am not sure if the hovers can be missed, if you were to have 2 consecutive hover in's or hover out's, then this code would mess up.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.