Show and hide posts using setTimeout

Question

I have made an effect that has the h1 move and then reveals p using .addClass, .removeClass and CSS3 transition. It works perfectly but I think I am repeating too much of the code and I think there may be a way to write it better. I need postShow and postHide to be different because I need setTimeout to be in postShow for timing. Please review the code below and give me any pointers.

jQuery(document).ready(function($){

    var post          = $('div.post');

    var postShow = function(item) {
            $(item).closest('a').find('img.postimg').addClass('imggrow');
            $(item).closest('a').find('h1').addClass('bannerhover');
            setTimeout(function(){
                $(item).closest('a').find('p.excerpt').addClass('phover');
            },500);
        };

    var postHide = function(item) {
            $(item).closest('a').find('img.postimg').removeClass('imggrow');
            $(item).closest('a').find('h1').removeClass('bannerhover');
            $(item).closest('a').find('p.excerpt').removeClass('phover');
    };

//-----------------------------------------------------------

    if ($(window).width() > 1000) {

        post.hover(function() {
            $(this).find('a').find('p.excerpt').show(0, function() {
                postShow(this);
            });
        },function() {
            $(this).find('a').find('p.excerpt').hide(0, function() {
                postHide(this);
            });
        });

    }

//--------------------------------
});

In the future it is better to write a title that describes what the code does instead of how you would like to improve it. I hope you get some good reviews! — Phrancis
– Phrancis, Commented Nov 16, 2015 at 21:40

Katana314 · Accepted Answer · 2015-11-19 15:48:03Z

Forewarning: My first post in Code Review! Here goes.

jQuery(document).ready(function($){

var post = $('div.post');

function postShowHide(showing) {
        var a = $(this).closest('a');
        a.find('img.postimg').toggleClass('imggrow', showing);
        a.find('h1').toggleClass('bannerhover', showing);
        function sethover(){
            a.find('p.excerpt').toggleClass('phover', showing);
        }
        if (showing) {
            setTimeout(sethover, 500);
        }
        else {
            sethover();
        }
    };

//-----------------------------------------------------------

if ($(window).width() > 1000) {

    post.hover(function() {
        $(this).find('a p.excerpt').show(0, postShowHide.bind(this, true));
    },function() {
        $(this).find('a p.excerpt').hide(0, postShowHide.bind(this, false));
    });

}

//--------------------------------
});

Couple things I made use of here: JQuery and similar frameworks have a toggle class variant that takes a Boolean parameter to either add or remove the class. This is useful for any situation that adds/removes the class since it means you only need one function, not two (as long as you can take a parameter)

To make the setTimeout(500) part work with that, I did end up separating it into a function, and conditional calling mechanism. This at least centralizes the logic, although it does elongate the method a bit.

If you're creating a callback that simply calls another function, often you can use Function.bind instead. The first argument is the target value of the this keyword, then the following arguments will be passed into the function. (If you're me and have an arbitrary contractual obligation to support Safari 5.0 for no reason, you can't use it but JQuery may have a similar method.)

Flambino · Accepted Answer · 2015-11-19 09:49:48Z

Firstly, you should add a class to the a element instead, since that's what all the other elements are contained in.

That way, you don't have to add/remove classes on all the child elements separately. You can just add/remove 1 class, and set up your CSS to style the child elements accordingly.

You also decouple your JS from the HTML to greater extent; now you can control things in just the CSS instead of having the HTML, the CSS, and the JS all concern themselves with the exact structure of your page.

Given your current code, however, I would suggest something like this:

jQuery(function($) { // shortcut for $(document).ready(...)
  function showPost() {
    var a = $(this).closest("a"); // get this once, use it thrice
    a.find('img.postimg').addClass('imggrow');
    a.find('h1').addClass('bannerhover');
    a.find('p.excerpt').delay(500).addClass('phover'); // note the delay() call
  }

  function hidePost() {
    var a = $(this).closest("a"); // same as above
    a.find('img.postimg').removeClass('imggrow');
    a.find('h1').removeClass('bannerhover');
    a.find('p.excerpt').removeClass('phover');
  }

  if ($(window).width() > 1000) {
    // No need to use hide/show with a timeout of zero, as far as I can tell.
    // Just pass the two functions directly
    $('div.post').hover(showPost, hidePost);
  }
});

With animations, there is actually commonly a reason to use a timeout-0. Basically, changing one value several times in one code block tells the browser to just skip to the last value. Changing a value twice in two separate callstacks signifies an actual user-visible change, and activates a transition property. Modern JavaScript also provides the setImmediate method which is faster. — Katana314
– Katana314, Commented Nov 19, 2015 at 15:37

Stack Exchange Network

Show and hide posts using setTimeout

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Show and hide posts using setTimeout

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions