5
\$\begingroup\$

I have a web layout that I created and then later wanted to add a dropdown menu to it. I found that my html was not quite optimum for that, so I wrote some jQuery that is now working quite nicely but I need this to be critiqued to see if and where I could have changed some things here.

My dropdown menu is the class "drop" which is an absolutely positioned div nested within the div main_right. I felt this was the best place to put it because I could simply position it right under the li item above it and hopefully avoid too many cross browser display issues. It is set to display:none via CSS.

HTML:

  <div id="menu">
      <ul>
          <li><a href="">1</a></li>
          <li><a href="">2</a></li>
          <li><a href="">3</a></li>
          <li><a href="">4</a></li>
          <li><a href="">5</a></li>
      </ul>
  </div>
  <div id="main">
        <div class="main_left_index">
        <!--some content-->
        <ul class="large">
          <li>who</li>
          <li>what</li>
          <li>when</li>
          <li>where</li>
        </ul>
        <img style="float:left;" src="example" alt="some other attributes"/>
        <p class="index"></p>
        </div>
        <div class="main_right">
            <div class="drop">
                <ul>
                  <li><a href="">this</a></li>
                  <li><a href="">that</a></li>
                  <li><a href="">then</a></li>
                  <li><a href="">things</a></li>
                </ul>
            </div>
        <img src="example" width="267" height="415" alt="example" />
        </div>
  </div>

And now the CSS just for my menu, to kind of justify why I went the route I did:

#menu{
    height:50px;
    width:100%;
    background:url(example);
    background-repeat:repeat-x;
    color:#FFF;
}
#menu ul{
    display:table;
    table-layout:fixed;
    padding:0;
    margin:0 auto;
    width:980px;
    height:50px;
}
#menu ul li{
    border-right:1px solid #CCCCCC;
    display:table-cell;
    float:none;
    list-style:none;
    font-family: Verdana, Arial, Helvetica, sans-serif;
    font-size:14px;
    line-height:50px;
    text-align:center;
}
#menu ul li a{
   display:block;
   width:100%;
   height:100%;
}
#menu ul li:first-child{
    border-left:1px solid #CCCCCC;
}
#menu ul li:hover{
    cursor:pointer;
    background-image:url(example);
}
#menu ul li:first-child:hover{
    text-decoration:underline;
}
#menu ul li:nth-child(2){
    width:177px;
}
#menu ul li:nth-child(3){
    width:168px;
}
#menu ul li:nth-child(4){
    width:180px;
}
#menu ul li:nth-child(5){
    width:220px;
}

And now the jQuery. If my divs were closer I would just have wrapped them in some type of container but I didn't think that was possible. So I essentially made a "buffer" with some mousemove variables and then set flags amongst the movement to control when the drop down should expand or collapse.

jQuery:

var dropDown ={

        start: function(){

            overDrop = false;
        $('#menu').on('mouseenter','ul li:nth-child(5)',function(){
               $('div.drop').slideDown('fast');
               //currentlyDown = true;
               overMenu = true;
           });
        $('#menu').on('mouseleave','ul li:nth-child(5)',function(){
                overMenu = false;
                if(overDrop == false && yMenu < 55){    
                    $('div.drop').slideUp('fast');
                    //currentlyDown = false;
                }
           });
        $('#menu').on('mousemove','ul li:nth-child(5)',function(e){
                  yMenu = e.pageY - $(this).offset().top + 10
           });    
        $('.main_right').on('mousemove','.drop',function(e){ 
                  yDrop = e.pageY - $(this).offset().top + 10             
           });


        $('.main_right').on('mouseenter','.drop',function(){
                  overDrop = true;
           });     
        $('.main_right').on('mouseleave','.drop',function(){
             if(overMenu == false && yDrop >15){
                $('div.drop').slideUp('fast');
             }
                 overDrop = false;

           });     
    }
}
$(document).ready(dropDown.start);

I think I need to add some X mouse coordinate checks to cover the lower corners...it is one inconsistency I am finding during further testing.

\$\endgroup\$
6
\$\begingroup\$

I've made a JS Bin from your posted code.

Initialization

When the pages is loaded, the dropdown menu starts in its expanded state. Is that what you want?

Usability, Positioning, and Semantic HTML

It's impossible to move from the "5" menu item to click on one of the items in the dropdown menu before it collapses. Perhaps it could work if the dropdown were positioned directly underneath "5", but you shouldn't rely on such fragile coincidences. The underlying problem is that semantically, the "this-that-then-things" dropdown menu should be a child of item "5" in the HTML.

<div id="menu">
  <ul>
    <li><a href="">1</a></li>
    <li><a href="">2</a></li>
    <li><a href="">3</a></li>
    <li><a href="">4</a></li>
    <li><a href="">5</a>
      <ul class="drop">
        <li><a href="">this</a></li>
        <li><a href="">that</a></li>
        <li><a href="">then</a></li>
        <li><a href="">things</a></li>
      </ul>
    </li>
  </ul>
</div>

CSS

You've hard-coded the width of menu items "2", "3", "4", "5" individually. If you need different widths, it would be easier to maintain if you specified the padding, so that each item's width scaled automatically according to the size of its content.

\$\endgroup\$
3
\$\begingroup\$

HTML

Your html is flawless (to me) pretty good. Good job.

CSS

Also completely flawless pretty good!. Well written, efficient, I wouldn't (personally) change a thing.

JQuery

You made a few formatting errors here. I'll refer you to this awesome JavaScript formatting guide by Douglas Crockford, and walk you through a few mistakes that I noticed ways that you might make your formatting a little more conventional.

The blank line here isn't suggested. Blank lines are, for the most part, best used at the bottom of function declarations (if at all).

var dropDown ={

        start: function(){

Instead:

var dropDown ={
        start: function(){

Uh oh... Global variable declarations. As a rule, you should keep variables in the proper scope.

Instead of

overDrop = false;

use:

var overDrop = false; 

or, at the top of the function:

var overDrop;

so that when you define overDrop it is defined in its proper scope.


You have this commented out, but the variable hasn't been declared, so I assume there's no use for keeping it.

//currentlyDown = true;

Your indentation here is not conventional. You should keep indentation to a limit of 4 spaces, except for when wrapping a line that is too long, in which case an 8 space indentation is recommended.

Instead of:

$('#menu').on('mousemove','ul li:nth-child(5)',function(e){
                  yMenu = e.pageY - $(this).offset().top + 10
           }); 

use:

$('#menu').on('mousemove', 'ul li:nth-child(5)', function (e) {
    yMenu = e.pageY - $(this).offset().top + 10
});

So, finally:

var overMenu, overDrop, yMenue, yDrop;
var dropDown = {
    start: function () {
        overDrop = false;
        $('#menu').on('mouseenter', 'ul li:nth-child(5)', function () {
            $('div.drop').slideDown('fast');
            overMenu = true;
        });
        $('#menu').on('mouseleave', 'ul li:nth-child(5)', function () {
            overMenu = false;
            if (overDrop == false && yMenu < 55) {
                $('div.drop').slideUp('fast');
            }
        });
        $('#menu').on('mousemove', 'ul li:nth-child(5)', function (e) {
            yMenu = e.pageY - $(this).offset().top + 10
        });
        $('.main_right').on('mousemove', '.drop', function (e) {
            yDrop = e.pageY - $(this).offset().top + 10
        });
        $('.main_right').on('mouseenter', '.drop', function () {
            overDrop = true;
        });
        $('.main_right').on('mouseleave', '.drop', function () {
            if (overMenu == false && yDrop > 15) {
                $('div.drop').slideUp('fast');
            }
            overDrop = false;
        });
    }
}
$(document).ready(dropDown.start);
\$\endgroup\$
  • 2
    \$\begingroup\$ As long as everyone involved in the maintenance of this code agrees that this is how they want it formatted, then there is no mistake. Nitpick about formatting when its inconsistent or significantly impacts readability, not because its in a style that differs from your own. \$\endgroup\$ – cimmanon Jul 26 '14 at 20:57
  • 1
    \$\begingroup\$ @cimmanon My answer is 100% based on best practices as demonstrated by a pretty reliable source. \$\endgroup\$ – Viziionary Jul 26 '14 at 22:42
  • \$\begingroup\$ meta.codereview.stackexchange.com/q/2151/9357 \$\endgroup\$ – 200_success Jul 27 '14 at 0:13

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.