5
\$\begingroup\$

This is the first time I've actually made my own jQuery script from scratch, because I couldn't find exactly what I was looking for. My code works how I want it to, although I don't know if I did it correctly. Just something about it is screaming to me, and although it works, I didn't do it properly. So I figured I would share what I have and see what you had to offer as a better way to accomplish this.

$(window).load(function() {
    $('.overlay').animate({
        opacity: 1,
        height: '0'
    }, 1000, function(){$(this).remove()});

    $("#frame").delay(300).animate({
        opacity: 1
    }, 300)
});

I have ran it though a JavaScript compressor which removed the ;s, and then through a "beautifier" for readability, but it still seems to work. I'm not even too sure how important those are in JavaScript. I'm a CSS guy and I know in CSS the last ; is not required, so figured I'd leave them out since it's working.

I set up an example of this in action here.

If anyone has any suggestions on my code as this being my first real attempt at jQuery, I would love some feedback.

\$\endgroup\$
2
  • \$\begingroup\$ To be honest, this is difficult to criticize, because you aren't saying what you are doing here and why - you are just executing two seemingly unrelated animations. What is the relation of the two DIVs with each other? Why does one animation start on load and the other on ready? Why does one animation run 1 second, the other 1.2 seconds? \$\endgroup\$
    – RoToRa
    May 27 '14 at 10:20
  • \$\begingroup\$ It's a page loading effect that I came up with for my external framed links, I was inspired to make it from themeforests and stumbleupons framed external links, the animation times those were just what looked the best timing wise. As for the load & ready... I have no idea lol kind of why I posted here to get some feedback, you can see it live at dirtrif.com/.secretstash/!PlayGround/r.php?l=http://… \$\endgroup\$
    – your mom
    May 27 '14 at 11:34
3
\$\begingroup\$

Interesting concept, and I do see some improvements that could be made.

First off though, it's true that the semicolons aren't necessary in JavaScript. It's a matter of preference really. If you think the people who will read your code would prefer semicolons, then use them where necessary. I suggets you try JSLint in the future!

  • I think that your animation is somewhat strange, but I'm not here to argue that! (The change in height of .overlay is what I'm talking about) I think that combination of animations is fine, and could be left as is.
  • Perhaps the one thing you could do: add some space to improve readability. Packing that last closure onto one line is just restrict breathing room, and it would be easier to quickly read it if it was spread out.
  • Your second animation could simply be:

    $('#frame').delay(300).fadeIn(300)
    

    You weren't the first one who thought about fading ;)

So my ideal function would be as follows:

$(window).load(function () {
    $('.overlay').animate({
        opacity: 1,
        height: 0
    }, 1000, function () {
        $(this).remove()
    });

    $("#frame").delay(300).fadeIn(300)
});
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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