2
\$\begingroup\$

I got tasked to develop an accordion-like function for a webpage.

  • The header will always be visible, while the main content (and footer) be styled as display: none;
  • At the end of the header, there's a button. Clicking on it will trigger my jQuery code.
  • The code should set <main> as display: block; and slowly scroll the screen to the beginning of the <main> element, setting top = <main>
  • Clicking on the same button again, will toggle <main> back into display: none; and scroll the screen to the beginning of iframe in the <header> element.

This is the jQuery code:

$(document).ready(function() {
    
    //jQuery: Basic Accordion
    $("#accordion").click(function() {
        /*Clicking on the button will toggle the content visibility*/
        $("main").slideToggle("slow", function() {
            /*Call scroll animation*/
            ScrollInto();
        });
  }); // END: Handler for .accordion ended

    //Scroll animation
    function ScrollInto() {
        //Animation speed
        var speed = 1500;
        
        if( $("main").is(":visible") ) { 
            $('html, body').animate({ scrollTop: $("main").offset().top }, speed, "swing"); 
        }
        else {   $('html, body').animate({ scrollTop: $("header > iframe").offset().top }, speed, "swing");   }
        return true;
    } //END: Scroll animation ended
}); // Handler for document.ready() ended.

The code works as intended. Would you please peer-review this code and tell me if this approach is appropriate and if there's something that can be improved / changed?

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Your code looks fine. First, there isn't much going on so it would be a little too early to think about optimizations or all other sorts of things, unless you experience a real problem.

But to make it more clear or simplify it, this is how I would recommend to do it: Cache HTML elements (selectors) into variables if you use them more than once. Don't repeat the same logic twice.

For the code of this size it doesn't make much difference but if it grows bigger it will be better if you reuse the code that you can and keep everything organized.

This is how I would have done it:

$(document).ready(function() {
    // 1. Cache HTML elements
    const $main = $('main');
    const $htmlBody = $('html, body');

    $("#accordion").click(function() {
        // 2. Remove anonymous function if you don't need to pass event object or call multiple functions
        $main.slideToggle("slow", ScrollInto);
    });

    function ScrollInto() {
        // 3. Use 'const' if you don't reassign the same variable
        const speed = 1500;
        // 4. Depending if <main> is visible store main, else iframe
        const $selector = $main.is(':visible') ? $main : $("header > iframe");
        // 5. Use $selector to get the top offset of the appropriate element
        $htmlBody.animate({
            scrollTop: $selector.offset().top
        }, speed, "swing");
    }
});

I just made a comment for every code change that I made. Let me explain:

1. Cache HTML elements Since you use element in multiple places it makes sense to reuse the variable that stores a reference to that HTML element wrapped inside jQuery object.

2. Remove anonymous function if you don't need to pass event object or call multiple functions

This is more of a suggestion, if you want to keep the code short, clean and simple, it's ok to just remove that anonymous function that was wrapping scrollInto() function call. The reasons where an anonymous function would be beneficial is if you need to execute more code, functions or whatever when the slideToggle action takes place, also if you need an event object then you would also need an anonymous function so you can pass a parameter there and use it or pass to other functions, but in your case you don't use it so it's all good.

3. Use 'const' if you don't reassign the same variable

Not sure if you are familiar with ES6+ syntax, the newer version of JavaScript that supports such keywords like: 'let' or 'const'. In this case I would suggest to use 'let' or 'const' instead of 'var'. Use 'let' when you know that the variable could be re-assigned or that will be re-assigned. Use 'const' when you know that the variable will never be re-assigned again. 'const' could potentially save you from re-assigning the variable's value unintendedly, you would always get an error in the console that would remind you. It also gives a sense of reliability that you use the same value and that it wasn't modified anywhere, although if you are new to ES6+ please dive deeper into it and learn more, although 'const' makes sure that variable can't be re-assigned, but if you hold an object there with some properties you can still modify it, it's a different thing that I won't go into further.

4. Depending if is visible store main, else iframe

Here we just create the variable that stores proper element, so we can reuse that variable.

5. Use $selector to get the top offset of the appropriate element

And here you just get the top offset of the element that was assigned. The thing was that in if/else you used the same .animate logic so just kinda simplified it without repeating it twice here, it looks a little cleaner.

Potential problems that could happen:

Let's say that your HTML structure changes and now you need to set slideToggle action to different element like 'div' or such, then make sure that you select only that one element specifically, or else you could run into not so obvious issue, since it could set slideToggle for all the elements that the jQuery found, not just one. So you can use id selector or select it specifically using jQuery methods like .first() or such depending on your case targeting with some pseudo selectors. Since you use 'main' element it is ok I presume. HTML specification states that - A document mustn't have more than one main element that doesn't have the hidden attribute specified.

Also before using .offset() method you could check that the selector that is assigned actually exists, like if ($selector.length) {} or such. Because if jQuery doesn't find an element that you were looking for - it will return you kinda an empty object then the offset() will return 'undefined' and then further blindly trying to get the top property you will get a TypeError, like: Cannot read property 'top' of undefined. So the proper checks before working with DOM elements could save you some time if you log those errors, because when something on the DOM doesn't work the way you expect it doesn't get clear just by viewing in the browser window you need to dive into the code obviously. But always be wary, working with DOM elements without you knowing if they really exist or were selected could lead to hard to find errors, in some cases jQuery really helps to prevent those nasty errors, like if you set an event to an element that wasn't found, unlike using .addEventListener or such you would get an error trying to set an event on anything other than Element Node, but you still gotta be careful.

So mostly those were suggestions what to look for or where to be careful. By having some defensive programming habits you will be able to find more potential problems that could arise and could even help you reduce or simplify the code.

I hope that some of my insights will be helpful :)

\$\endgroup\$
2
  • \$\begingroup\$ Correct me if I am wrong. Isn't inappropriate to name a constant/variable with the first character to be a number or a special character, like $. Specially in jQuery? \$\endgroup\$
    – Omar
    Jun 11, 2021 at 3:25
  • \$\begingroup\$ Yeah the variable name can't start with a number, but it can start with a dollar sign. One of the rules are: Variable names must begin with a letter, an underscore (_) or a dollar sign ($). Also in jQuery community or when writing jQuery plugins is quite known variable naming convention to start variables with $ for selected elements, in this context the $selector would indicate that you're storing some kind of element wrapped inside $, like: $('div') for example. stackoverflow.com/questions/10204606/… \$\endgroup\$
    – Erasus
    Jun 11, 2021 at 3:39

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.