HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Show and hide posts using setTimeout

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
showsettimeoutusingandpostshide

Problem

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);
            });
        });

    }

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

Solution

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);
  }
});

Code Snippets

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);
  }
});

Context

StackExchange Code Review Q#110953, answer score: 3

Revisions (0)

No revisions yet.