6
\$\begingroup\$

This code works as I intended, but I am definitely a JS noob (C++ background), so I'm not sure if this is the best way to do it (for example using .each instead of some kind of a for loop was completely new to me).

https://github.com/TaylorHuston/menuBreak/blob/master/menuBreak.js

function menuBreak() {

var windowWidth = $(window).width();

var menuLength = 0;

$("#navUl > li").each( function (index, element) {
    menuLength += $(this).width();
});

if( menuLength > windowWidth*.9 ) {
    var newWidth= menuLength*.7;
    $('#navUl').css({"maxWidth":newWidth});
} else {
    $('#navUl').css({"maxWidth":windowWidth});
}

}

And in my HTML, not sure if this is the 'best' way to call it

<script>
  $(document).ready(function() {
    menuBreak();
  });

  $(window).resize(function() {
    menuBreak();
  });
</script>
\$\endgroup\$
3
\$\begingroup\$

From a once over:

  • You could have used a loop instead of .each(), it would be marginally faster
  • Your indenting is off, I assume you had trouble pasting the code
  • .9 and .7 are magic constants, you should declare these with var and a good name
  • I would use a ternary for this:

    if {
        var newWidth= ;
        $('#navUl').css({"maxWidth":newWidth});
    } else {
        $('#navUl').css({"maxWidth":});
    }
    

    could be

    var navWidth = ( menuLength > windowWidth *.9 ) ? menuLength *.7 : windowWidth;
    $('#navUl').css({"maxWidth":navWidth});
    

    of course with properly named constants this might stretch too much to be easily readable, up to you.

  • With jQuery, the most common way to trigger on ready is

    $( function(){
      //Do Something
      menuBreak();
    });
    

    now, since $() expects a function, we might as well pass menuBreak immediately:

    $( menuBreak );
    

    The same things goes for resize:

    $(window).resize( menuBreak );
    
\$\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.