4
\$\begingroup\$

I am making a dropdown menu in JavaScript for my website, and recently I decided I wanted to make it a plugin (as in, upload it on the web and make it compatible with other plugins). I am new to making plugins (and JavaScript) and I was wondering what you think of my code. My goal with the plugin is so that all the user needs to do is have the proper markup in their HTML along with some CSS, then include the script in their header, and it should make a functioning dropdown menu.

Here is the required markup:

<ul class="dropdown_container">
    <li class="super_tab"><a>Menu</a></li>

    <li><ul class="dropdown hidden">
        <li class="sub_tab"><a>Cater</a></li>
    </ul></li>
</ul>

I won't include the CSS because it is unimportant.

(function() {
    $(function Main() {

        this.setup = $(function() {
            self = this;
            this.cont_divs = this.getContDivs();
            this.cont_objs = this.defContObjects(this.cont_divs);

            this.mouseHovers();
        });

        this.mouseHovers = function() {
            $(document).on('mouseenter', '.super_tab',
                function() {
                    var parent = this.parentNode;
                    var cont = self.getContObj(parent, self.cont_objs);
                    cont.dropdown.show();
                }
            ).on('mouseleave', '.dropdown_container',
                function() {
                    var cont = self.getContObj(this, self.cont_objs);
                    cont.dropdown.hide();
                }
            ).on('mouseenter mouseleave', '.dropdown_container',
                function() {
                    var cont = self.getContObj(this, self.cont_objs);
                    cont.setDimensions();
                }
            );
        }

        this.getContObj = function(other_div, cont_objs) {
            for (var i = 0; i < cont_objs.length; i++) {
                if (other_div === cont_objs[i].div) {
                    return cont_objs[i];
                } 
            };
        }

        this.getContDivs = function() {
            var divs = document.getElementsByClassName("dropdown_container");
            return divs;
        }

        this.defContObjects = function(sl_show_divs) {
            var cont_objs = [];

            for (var i = 0; i < sl_show_divs.length; i++) {
                var cont = new DropdownContainer(sl_show_divs[i]);
                cont_objs.push(cont);
            };

            return cont_objs;
        }

    });


    function DropdownContainer(div) {

        this.div = div;
        this.children = $(this.div).find('*');

        for (var i = 0; i < this.children.length; i++) {
            var child = this.children[i];

            if ($(child).hasClass("dropdown")) {
                this.dropdown = new Dropdown(child);
            } else if ($(child).hasClass("super_tab")) {
                this.super_tab = new SuperTab(child);
            }
        };

        this.setDimensions = function() {
            var drop_height = this.dropdown.getHeight();
            var super_tab_height = this.super_tab.getHeight();
            var new_height = super_tab_height + drop_height;

            if (this.dropdown.isVisible()) {
                this.setHeight(new_height);
            } else {
                this.setHeight(super_tab_height);
            }
        }

        this.setHeight = function(height) {
            this.div.style.height = height + 'px';
        }
    }


    function Dropdown(div) {

        this.div = div;
        this.children = this.div.childNodes;
        this.visible = false;

        this.show = function() {
            this.defVisibilityProps("block", "dropdown visible");
            this.visible = true;
        }

        this.hide = function() {
            this.defVisibilityProps("none", "dropdown hidden");
            this.visible = false;
        }

        this.getHeight = function() {
            return $(this.div).outerHeight();
        }

        this.isVisible = function() {
            return this.visible;
        }

        this.defVisibilityProps = function(display, class_names) {
            this.div.style.display = display; // Changing attribute "visibility" does not work as well. Use display.
            this.div.className = class_names; // Needed to tell what state the dropdown is currently in.
        }

    }

    function SuperTab(div) {

        this.div = div;
        this.children = this.div.childNodes;

        this.getHeight = function() {
            return $(this.div).outerHeight();
        }
    }
}());
\$\endgroup\$
1
\$\begingroup\$

Your HTML is badly structured. The dropdown menu items shouldn't be in a separate li. Something like this would be better:

<ul class="dropdown_container">
    <li class="super_tab">
        <a>Menu</a>
        <ul class="dropdown hidden">
            <li class="sub_tab"><a>Cater</a></li>
        </ul>
    </li>
</ul>

  • You use $( ... ) (document ready) twice nested inside each other.

  • The function Main is used anonymously, so it doesn't need a name.

  • Main also isn't called as a constructor function or a method, so this inside it will reference window and all the functions you define in it (setup, mouseHovers, getContObj, all are written into the global namespace.

  • The variable self is also in the global namespace.

  • Any special reason you use getElementsByClassName instead of jQuery? It's not wrong, and most likely faster - if the browser supports it -, but it seems strange to mix jQuery and DOM queries.

  • In a similar vein: jQuery provides .each() to loop over elements instead of for loops.

  • I just realize there are more mixtures of jQuery and non-jQuery. (DOM's .childNodes instead of jQuery's .children, defVisibilityProps instead of jQuery's .toggle(), etc.)

  • .find('*') doesn't return all children, it returns all decedents. For the children use .find('> *') or just .children(). If you actually want the decedents, then rename the variable.

The is more, but I don't have the time right now.

\$\endgroup\$
  • \$\begingroup\$ Ok, what you said about the main function is totally correct and I fixed that. Though, from the html you suggest, it makes it seem as if the hidden dropdown is a part of the super tab, which it isn't. It is a part of the dropdown menu. The dropdown (the container for all the sub tabs) should be treated as a part of the dropdown menu, not a part of the super tab. The dropdown menu (which I incorrectly named dropdown_container) is the whole thing, including the super tab and the sub tabs. \$\endgroup\$ – Robbie Aug 11 '15 at 17:44
  • \$\begingroup\$ Also, about what you said about mixing jquery and javascript, this article says its totally normal to mix the two: programmers.stackexchange.com/questions/166273/…. Though, your last point about the children variable is correct. I indeed wanted all the descendants, not the children of it. \$\endgroup\$ – Robbie Aug 11 '15 at 17:46
  • \$\begingroup\$ Why do you think that the items should not be in a seperate li? @RoToRa \$\endgroup\$ – Robbie Aug 12 '15 at 0:21
  • \$\begingroup\$ The HTML structure is just an example. To be honest I didn't quite understand your code, mainly because the class names super_tab and sup_tab are completely meaningless. The dropdown shouldn't be in a separate li because the structure of a list means that each item is its separate "entity" on the same level, which a dropdown menu is not. \$\endgroup\$ – RoToRa Aug 12 '15 at 6:52
  • \$\begingroup\$ As I wrote, it's not wrong to mix jQuery and direct DOM access, but personally I find it really confusing and makes the code hard to read. Either you use jQuery or you don't. If you don't like jQuery (which I understand) because it's too complex/bloated/whatever, don't use it, or look for an alternative. \$\endgroup\$ – RoToRa Aug 12 '15 at 6:58
1
\$\begingroup\$

I noticed that you could shorten this up a bit

    this.getContDivs = function() {
        var divs = document.getElementsByClassName("dropdown_container");
        return divs;
    }

and make it this instead:

this.getContDivs = function() {
    return document.getElementsByClassName("dropdown_container");
}

You don't need to create a variable just to return the value, just return what you want to return here.

\$\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.