Return to Answer

• 1

• 641
• 4
• 9

• 641
• 4
• 9

For someone to straight-up call this code messy is, I think, uncalled for.

I agree with mystrdat's answer suggesting the lack of semantics is a big deal. This is low-hanging fruit for 'optimizing' the markup. However, I think you have some larger issues.

The site is actually buggy.

When I click and drag the 'header title' elements onto one another the relayout is not what I would expect. It's not difficult to get them all in one column or moving around almost unpredictably. I'm almost certain this is not the intended behavior, which makes it a bug and a pretty big one. I think this is the biggest problem as it simply doesn't work. IMHO, this is far more significant than whether the code is messy or optimized.

When I tested in FF, the widget_header_title elements are too wide and this causes the widget_configure_button elements to be pushed down, making the widget_header elements to be twice as tall as you're going for (according to how it renders in Chrome). Changing the markup and CSS to the following fixes this, as far as I can tell:

HTML

<div class="widget_header">
 <div class="widget_header_icon">( i )</div>
 <div class="widget_configure_button">(c)</div>
 <div class="widget_header_title">header title</div> <!-- now 3rd item -->
 <div class="dropdown">
 <div>icon Default</div>
 </div>
</div>

CSS

/* taking out float and hard-coded width */
.widget_header_title {
 padding: 10px;
 cursor: move;
 background: #CCC;
 overflow: hidden;
}

However, this breaks breaks your dropdown code because the dropdown code relies on the .next() jQuery method. Relying on precise ordering of sibling elements in the DOM can yield brittle code. Maybe change this so the button and corresponding dropdown element are both inside a container element. This will be more stable, maintenance wise. An alternative is to add a data-target attribute (or something similar) on the button and set the value to a selector that will target the dropdown to toggle. The problem with this approach it generally requires the use of IDs.

Container:

<div class="dropdown_group">
 <div class="menu_button">menu</div>
 <div class="dropdown_left">
 <div>icon Default 2</div>
 <div>icon Reports 2</div>
 <div>icon Other 2</div>
 </div>
<div>

Related via data-target:

<div class="menu_button" data-target="#dropdown1">menu</div>
<div id="dropdown1" class="dropdown_left">
 <div>icon Default 2</div>
 <div>icon Reports 2</div>
 <div>icon Other 2</div>
</div>

Your JS relies a lot on dispatching UI events to trigger other code you've written. This should absolutely be avoided. It's difficult to read and is inefficient. If there is an operation that's executed by a click-handler that you need to execute, make it available as a function and call it. If that won't work or is not an option, then take a different approach.

Combining the two notes, your dropdown code could be written something like:

HTML

<div class="menu_wrap">
 <button class="menu_button">menu</button>
 <ul class="dropdown_left">
 <li><a href="#some_link">icon Default 2</a></li>
 <li><a href="#some_link">icon Reports 2</a></li>
 <li><a href="#some_link">icon Other 2</a></li>
 </ul>
</div>

CSS

.menu_wrap {
 position: relative;
}
.menu_wrap.open > .dropdown_left {
 display: block;
}

JS

$('.menu_wrap').on('click.dropdownToggler', function(e){
 var $this = $(this);
 if ($this.hasClass('open')) {
 $this.removeClass('open');
 } else {
 $('.menu_wrap.open').removeClass('open');
 $this.addClass('open');
 }
 return false;
});
$(document).on('click.dropdownCloser', function(e) {
 $('.menu_wrap.open').removeClass('open');
});

Generally speaking, you may want to use CSS to position your dropdowns instead of JavaScript. Wrapping the dropdown elements in something like menu_wrap as noted above will make it possible to only use CSS for positioning the menus.

.menu_wrap {
 position: relative;
}
.dropdown_left {
 position: absolute;
 top: 100%;
 left: 10px;
 /* etc. */
}

In your CSS, there are a lot of situations where you have a hardcoded width. I try to avoid this because it makes for brittle layouts (like the widget_header_title elements). For example:

.inner_footer {
 width: 983px;
 margin: auto;
 padding: 10px;
 background: #999;
}

Can be written as:

.inner_footer {
 margin: 0 10px;
 padding: 10px;
 background: #999;
}

For this to be centered you need a containing element that has the width set, but this container can contain the rest of your site. This way, you only have the width set in one place:

HTML

<body>
 <div class="container">
 <section class="notification">
 <div class="inner_notification">
 notification
 </div>
 </section>
 
 <section class="header">
 <div class="inner_header">
 header
 </div>
 </section>
 
 <!-- etc -->
 </div><!-- .container -->
</body>

CSS

.container {
 width: 1003px;
 margin: 0 auto;
}

There are tradeoffs, as this doesn't have the various wide bands of color that fill the margins. For that, you would just want to add the container class to each of your ourtermost wrapping elements. But, you still have the width set in only one place in your CSS.

 <div class="notification container">
 <div class="inner_notification">
 notification
 </div>
 </div>

Per mystrdat's remark, those wrapping elements (.notification) should really be <section> elements. And, the menu triggers (configure_button, widget_configure_button and menu_button) should be <button> elements. Your dropdowns should be <ul> elements. Your footer could be a <footer> element. That would all make it more "semantic" which is definitely a good thing and generally helps when it comes to what other developers think of your markup (since it was mentioned in the OP).