2
\$\begingroup\$

I have this working code for a toggle-item inside a Bootstrap dropdown-menu. See demo here.

enter image description here

I suspect that it should be possible to improve it :

  • in order to have a shorter code and avoid all the (...).css('visibility','hidden'); code redudancy... Is it really like this that I should move the tick icon on the selected item of the Private/Public menu?

  • are the inline HTML style="..." a problem here and there ?

  • more generally, as I learned JS by myself (+ with help of people on SO as well!), are there some good pratice that I should use here and there in this code ? Naming for the class/id ? Replace the use of class by id ? some other things ?

<!DOCTYPE html>
<html lang="en">

<head>
  <meta http-equiv="content-type" content="text/html; charset=UTF-8">
  <script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1.8/jquery.min.js"></script>
  <link href="http://maxcdn.bootstrapcdn.com/bootstrap/3.2.0/css/bootstrap.min.css" rel="stylesheet">
  <script src="http://maxcdn.bootstrapcdn.com/bootstrap/3.2.0/js/bootstrap.min.js"></script>
  <style>
  #privacy-button {
    margin: 3px 0 10px 20px;
    line-height: 10px;
  }
  #privacy-menu {
    margin-top:-15px; 
    margin-left:20px;
  }
  </style>
</head>

<body>
  <div class="navbar navbar-default navbar-fixed-top" role="navigation">
    <div class="container">
      <ul class="nav navbar-nav">
        <li><a href="#">Home</a> </li>
        <li class="dropdown"> <a href="#" class="dropdown-toggle" data-toggle="dropdown">Settings<span class="caret"></span></a>
          <ul class="dropdown-menu" role="menu" style="min-width: 210px;">
            <li><a href="#">Rename</a> </li>
            <li><a href="#">Delete</a> </li>
            <li class="dropdown-inner">
              <button type="button" class="btn btn-default" id="privacy-button"><span id="privacy-icon" class="glyphicon glyphicon-lock" style="font-size:10px;"></span>&nbsp<span id="privacy-text">Private project</span> &nbsp<span class="caret"></span> </button>
              <ul class="dropdown-menu" id="privacy-menu">
                <li><a href="#" id="private-menuitem"><span id="private-tick" class="glyphicon glyphicon-ok" style="font-size:10px;">&nbsp</span><span class="glyphicon glyphicon-lock" style="font-size:10px;"></span>&nbsp Private</a> </li>
                <li><a href="#" id="public-menuitem"><span id="public-tick" class="glyphicon glyphicon-ok" style="font-size:10px; visibility:hidden;">&nbsp</span><span class="glyphicon glyphicon-globe" style="font-size:10px;"></span>&nbsp Public (read-only for others)</a> </li>
              </ul>
            </li>
          </ul>
        </li>
        <li><a href="#contact">Contact</a> </li>
      </ul>
    </div>
  </div>
  <script>
  $('.dropdown button').click(function(e) {
    e.stopPropagation();
  });
  $('#privacy-button').click(function(e) {
    e.stopPropagation();
    $('.dropdown-inner').toggleClass('open').trigger('shown.bs.dropdown');
  });

  $('.dropdown').on('hide.bs.dropdown', function(e) {  $('.dropdown-inner').removeClass('open');} );

  $('#public-menuitem').click(function(e) { 
    e.stopPropagation(); 
    $('#privacy-text').text('Public project'); 
    $('#privacy-icon').toggleClass('glyphicon-lock').toggleClass('glyphicon-globe');
    $('#public-tick').css('visibility','visible'); 
    $('#private-tick').css('visibility','hidden');  
  });
  $('#private-menuitem').click(function(e) { 
    e.stopPropagation(); 
    $('#privacy-text').text('Private project'); 
    $('#privacy-icon').toggleClass('glyphicon-lock').toggleClass('glyphicon-globe');
    $('#public-tick').css('visibility','hidden'); 
    $('#private-tick').css('visibility','visible');  
  });
  </script>
</body>

</html>
\$\endgroup\$

1 Answer 1

Reset to default
4
\$\begingroup\$

It'd definitely be nice to get rid of the inlined style attributes. Since you've already ID'ed the sub-menu pretty extensively, it's pretty easy to add this to the styles you already have. E.g.

#privacy-menu .glyphicon {
  font-size: 10px;
}

gets rid of the inlined font-size styles.

You can also do the toggling by adding a class to the #privacy-menu, which then determines the styles for the nested items in the list, so you only have to set a class on 1 element.

However, for this to work for some things I'll get to later, it's actually the .dropdown-inner element that should be IDed (I've just moved the privacy-menu ID, but you could use another ID, instead of moving an existing one.

For instance,

/* private state (default) */
#privacy-menu #private-tick { visibility: visible }
#privacy-menu #public-tick { visibility: hidden }

/* public state (the menu gets a "public" class) */
#privacy-menu.public #private-tick { visibility: hidden }
#privacy-menu.public #public-tick { visibility: visible }

This is less error-prone than messing with the two options individually, and it makes it easier to read the state (e.g. if $('#privacy-menu').hasClass('public') returns true, the "public" option is set; otherwise the "private" option is set). A clear binary state. When you set the two options individually, you have to make sure that you don't accidentally set them both to be visible or hidden at the same time.

Incidentally, that's also why I'd avoid a line like this:

$('#privacy-icon').toggleClass('glyphicon-lock').toggleClass('glyphicon-globe');

because toggleClass is less explicit about what will happen than addClass and removeClass. If, for any reason, neither of the glyphicon-* classes are present, then toggleClass will add both of them (or remove both of them if both are present).

However, setting the correct lock/globe icon via a single class on the menu is a little more involved. You can either:

  1. Continue to set the correct class via JavaScript (just use add/removeClass() instead), but that sort of defeats the point of having a single class on the menu determine all the styling.

  2. You can add 2 icons in the HTML, and hide/show (using display, not visibility) them in CSS much like how the ticks are handled in the code above.

  3. You can simply give the icon a glyphicon base class, but not give it any of the specific icon classes. Then go straight to the Bootstrap source and look at how the glyphicon-globe/glyphicon-lock classes work, and do that yourself in your CSS (spoiler: they use content and set a certain character, since the glyphicons are defined as a font)

I'd choose option 3, although it does duplicate code from Bootstrap. Something like:

/* private state (default) */
#privacy-menu #privacy-icon:before { content: "\e033" } /* same as glyphicon-lock */

/* public state */
#privacy-menu.public #privacy-icon:before { content: "\e135" } /* same as glyphicon-globe */

Last up is the text for #privacy-text. This could be set with CSS to, but it'd be a poor use of CSS (which is for presentation, not content). It'd also be hard to localize if the page has to be translated. Instead, that's one thing I'd still set via JS. However, I'd do so without having the text strings in the code (because JS isn't for content either, but for behavior). Instead I'd add those to the HTML as a data-description (or similar) attributes on the links.

Then all we need it to watch for clicks on those links and set/unset 1 class, and set 1 piece of text:

$('#privacy-menu a').click(function(e) { 
  e.stopPropagation();
  // check what link was clicked and update the menu accordingly
  if(/public/.test(this.id)) {
    $('#privacy-menu').addClass('public');
  } else {
    $('#privacy-menu').removeClass('public');
  }
  // update the text
  $('#privacy-text').text($(this).data('description'));
});

The snippet below does all of the above. (Note: it looks a bit different unless you run it full page because the preview is so narrow that Bootstrap does its responsive design thing.)

$('#privacy-button').click(function(e) {
  e.stopPropagation();
  $('#privacy-menu').toggleClass('open').trigger('shown.bs.dropdown');
});

$('.dropdown').on('hide.bs.dropdown', function(e) {
  $('#privacy-menu').removeClass('open');
});

$('#privacy-menu a').click(function(e) { 
  e.stopPropagation();
  if(/public/.test(this.id)) {
    $('#privacy-menu').addClass('public');
  } else {
    $('#privacy-menu').removeClass('public');
  }
  $('#privacy-text').text($(this).data('description'));
});
#settings-menu {
  min-width: 210px;
}

#privacy-button {
  margin: 3px 0 10px 20px;
  line-height: 10px;
}

#privacy-menu .dropdown-menu {
  margin-top:-15px; 
  margin-left:20px;
}

#privacy-menu .dropdown-menu .glyphicon {
  font-size: 10px;
}

/* default state (private) */
#privacy-menu #private-tick { visibility: visible }
#privacy-menu #public-tick { visibility: hidden }
#privacy-menu #privacy-icon:before { content: "\e033" }

/* public state */
#privacy-menu.public #private-tick { visibility: hidden }
#privacy-menu.public #public-tick { visibility: visible }
#privacy-menu.public #privacy-icon:before { content: "\e135" }
<script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1.8/jquery.min.js"></script>
<link href="http://maxcdn.bootstrapcdn.com/bootstrap/3.2.0/css/bootstrap.min.css" rel="stylesheet">
<script src="http://maxcdn.bootstrapcdn.com/bootstrap/3.2.0/js/bootstrap.min.js"></script>

<body>
  <div class="navbar navbar-default navbar-fixed-top" role="navigation">
    <div class="container">
      <ul class="nav navbar-nav">
        <li class="dropdown"> <a href="#" class="dropdown-toggle" data-toggle="dropdown">Settings<span class="caret"></span></a>
          <ul id="settings-menu" class="dropdown-menu" role="menu">
            <li><a href="#">Rename</a> </li>
            <li><a href="#">Delete</a> </li>
            <li class="dropdown-inner" id="privacy-menu">
              <button type="button" class="btn btn-default" id="privacy-button">
                <span id="privacy-icon" class="glyphicon">&nbsp;</span>
                <span id="privacy-text">Private project</span>
                <span class="caret"></span>
              </button>
              <ul class="dropdown-menu">
                <li>
                  <a href="#" id="private-menuitem" data-description="Private project">
                    <span id="private-tick" class="glyphicon glyphicon-ok">&nbsp;</span>
                    <span class="glyphicon glyphicon-lock"></span>
                    Private
                  </a>
                </li>
                <li>
                  <a href="#" id="public-menuitem" data-description="Public project">
                    <span id="public-tick" class="glyphicon glyphicon-ok">&nbsp;</span>
                    <span class="glyphicon glyphicon-globe"></span>
                    Public (read-only for others)
                  </a>
                </li>
              </ul>
            </li>
          </ul>
        </li>
      </ul>
    </div>
  </div>
</body>

\$\endgroup\$
3
  • \$\begingroup\$ Thanks @Flambino! Exactly the kind of review I was looking for! I learned a lot of thing here! \$\endgroup\$
    – Basj
    Commented Oct 12, 2014 at 21:37
  • \$\begingroup\$ you can see the result of your help @Flambino on this website project : bigpicture.bi/demo ... You'll need to create an account (no need to put your real email) to see the "Privacy menu" in action... I'm very open to all kind of code-review on other parts of the code of the website too :) See you! \$\endgroup\$
    – Basj
    Commented Oct 17, 2014 at 20:19
  • \$\begingroup\$ May I ask you some advices on codereview.stackexchange.com/questions/71718/… @Flambino ? \$\endgroup\$
    – Basj
    Commented Dec 5, 2014 at 0:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.