7
\$\begingroup\$

I have the following code to get the ID of a <a href="#ID"> </ a> and go to their respective div <div id="" />:

$('a[href=#certificados]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#certificados').offset().top }, 1000);
});

$('a[href=#team]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#team').offset().top }, 1000);
});

$('a[href=#house]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#house').offset().top }, 1000);
});

$('a[href=#contact]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $('#contact').offset().top }, 1000);
});

How could I optimize the code?

\$\endgroup\$

migrated from stackoverflow.com Jan 14 '14 at 13:57

This question came from our site for professional and enthusiast programmers.

11
\$\begingroup\$

Get the "respective" div in a more general way from the link's href attribute:

$('a[href=#certificados], a[href=#team], a[href=#house], a[href=#contact]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(this.hash).offset().top }, 1000);
    // this.hash would be equivalent to $(this).attr("href") in your case
});

Probably you also can use a much better selector now - maybe selecting those links by a common class.

\$\endgroup\$
  • 3
    \$\begingroup\$ Bad thing about this solution is maintaining the list of items and the slower look-up. You would be better off using a common classname. \$\endgroup\$ – epascarello Jan 14 '14 at 14:25
  • 3
    \$\begingroup\$ @epascarello: That's what I suggested in the last sentence :-) \$\endgroup\$ – Bergi Jan 14 '14 at 14:36
  • \$\begingroup\$ Just hoping the OP saw it. :) \$\endgroup\$ – epascarello Jan 14 '14 at 14:46
5
\$\begingroup\$

If you want to make the code DRYer, you can do

['#certificados', '#team', '#house', '#contact'].forEach(function(anchor){
    $("a[href="+anchor+"]").click(function(e){
        e.preventDefault();
        $("html, body").animate({ scrollTop: $(anchor).offset().top }, 1000);
    });
})

If all internal links are to be handled this way, do

$('a[href^="#"]').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(this.hash).offset().top }, 1000);
});
\$\endgroup\$
  • \$\begingroup\$ Why a forEach? jQuery supports multiple elements in a selector! \$\endgroup\$ – epascarello Jan 14 '14 at 14:01
  • \$\begingroup\$ @epascarello to not compute n and to be dryer on the a[href=...] thing \$\endgroup\$ – Denys Séguret Jan 14 '14 at 14:01
  • \$\begingroup\$ Maybe rename n to anchor ?, +1 \$\endgroup\$ – konijn Jan 14 '14 at 14:04
  • 1
    \$\begingroup\$ this.href does not work \$\endgroup\$ – Bergi Jan 14 '14 at 14:14
  • 1
    \$\begingroup\$ this.href returns the full URL, You would need to use this.getAttribute("href") or $(this).attr("href"); or hash jsfiddle.net/7439C/1 \$\endgroup\$ – epascarello Jan 14 '14 at 14:22
3
\$\begingroup\$

You can make an extension, that will let you specify what link scrolls to where, without creating a hard dependancy between the href attribute and the target identity:

$.fn.scrollTo = function(target){
  return this.click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(target).offset().top }, 1000);
  });
};

$('a[href=#certificados]').scrollTo('#certificados');
$('a[href=#team]').scrollTo('#team');
$('a[href=#house]').scrollTo('#house');
$('a[href=#contact]').scrollTo('#contact');
\$\endgroup\$
  • \$\begingroup\$ Nice, but this doesn't optimize it at all. \$\endgroup\$ – Silviu Burcea Jan 14 '14 at 22:36
  • \$\begingroup\$ @SilviuBurcea: Funny that you should single out this answer and point out that it won't optimise the code (and I assume that you only mean optimise for speed, not any other aspect), as neither of the previous answers does that either... \$\endgroup\$ – Guffa Jan 14 '14 at 23:13
0
\$\begingroup\$

Add a class to the anchors, like scrollable.

$('.scrollable').click(function(e){
    e.preventDefault();
    $("html, body").animate({ scrollTop: $(this.hash).offset().top }, 1000);
});

Each is called under the hood, no crazy functions, just KISS.

\$\endgroup\$

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.