2
\$\begingroup\$

Here is a JSFiddle of an accordion menu that i've done. I have used :before to add the icons.

My question is, is there a better way to Add different icon to each li item?

My code is as follows:

HTML Structure:

    <ul id="accordion">
  <li><div id="profile">Profile</div></li>
  <li><div id="messages">Messages</div>
    <ul class="submenu">
      <li>Inbox</li>
      <li>Sent</li>
      <li>Drafts</li>
    </ul>  
  </li>
  <li><div id="settings">Settings</div></li>
  <li><div id="logout">Logout</div></li>
</ul>

CSS:

    body {
background-image: url(http://subtlepatterns.subtlepatterns.netdna-cdn.com/patterns/tiny_grid.png);
}

ul{
  list-style-type:none;
  padding:0;
  margin:0;
}

#accordion{
  margin: 20% auto 80% auto;
  width:200px;
  height:auto;
  background-color:#212830;
  border-radius:5px;
  color: #BECEDB;
  font-family: 'Maven Pro', sans-serif;
  border: 1px solid #191E24;
  box-shadow: 0px 0px 5px 2px #666666;
}

#accordion > li > div{
  background-color:#383C48;
  font-weight:700;
  -moz-box-shadow:inset 0 1px 1px -1px #A1B3C9;
  -webkit-box-shadow:inset 0 1px 1px -1px #A1B3C9;
  box-shadow:inset 0 1px 1px -1px #A1B3C9;
  border-bottom:1px solid #191E24;
}

#accordion > li > div, #accordion > li > ul > li{
  padding:10px 15px;
} 

#accordion li{
  cursor:pointer;
}

#accordion > li:first-child >div{
  border-radius: 5px 5px 0 0;
}

#accordion > li:last-child >div{
  border-radius: 0 0 5px 5px;
}

#profile:before,
#messages:before,
#settings:before,
#logout:before,
.submenu>li:before
{
  font-family:FontAwesome;
}

#profile:before{
  content:"\f007  ";
}
#messages:before{
  content:"\f0e0";
}

#settings:before{
  content:"\f013";
}

#logout:before{
  content:"\f08b";
}

.submenu li:before{
  content:"\f0da"
}

#accordion *:before{
  margin-right:10px;
}

.submenu li{
  margin-left:10px;
}

JS code: (No problem with this)

 $("#accordion > li > div").click(function(){
  if(false == $(this).next().is(':visible')){
        $('#accordion ul').slideUp(400);
    }
  $(this).next().slideToggle(300);
 });

$('#accordion ul:eq(0)').show();

Also I would like to know if there are other areas this can be optimized.
Thank you

\$\endgroup\$
  • 1
    \$\begingroup\$ It is good that you linked to a demo of the code, but we can only review code that's inside the question itself. Please edit your question to include the relevant code. \$\endgroup\$ – amon Jan 29 '14 at 10:42
4
\$\begingroup\$

First things first: You have a navigation but refuse to use anchor tags. Why? This is potentially harmful. How are you going to navigate if you don't have links? The only possible way would be completely relying on JavaScript. All users with disabled JavaScript have a useless list of words instead.

HTML:

  • Use a tags instead of the div's in your HTML (Use <a href="#"> to test this)
  • I'd suggest using classes instead of ID's in this scenario as well. The lower specificity of classes will ease your live later. Overwriting an ID instead could be hard

CSS:

  • If you use actual links for your navigation, there is no need for cursor: pointer; on list-items.
  • If you want the icons to be clickable as well, add the pseudo-elements to the links instead of the list-items
  • Don't reset the general styling for lists on the type selector ul. Doing this makes you unable to use lists as regular lists in text etc. You can reset the styles for the navigation on #accordion
  • Adding a class accordion-item to your list-items could help you. You don't selecting like this: #profile:before, #messages:before, #settings:before...

You have some things to consider now. There are more things to improve, but I'd like to see a new question for an updated navigation with the given advice.

\$\endgroup\$
  • \$\begingroup\$ Oops, I forgot to mention, my plan was to wrap Message, Profile ... etc each with anchor tags. Its not that I want to avoid them. Just didn't use here because I was mostly focusing on the looks. \$\endgroup\$ – md1hunox Jan 29 '14 at 11:32
  • 4
    \$\begingroup\$ If my advice helped you, it would help to have a new question with the updated markup/CSS. Don't edit the anchors into this question, because this would invalidate my answer and potential others. \$\endgroup\$ – kleinfreund Jan 29 '14 at 11:34
3
\$\begingroup\$

I'm going to answer this question:

is there a better way to Add different icon to each li item?

The short answer is, yes. The better way is to use the classes that FontAwesome provides out of the box, rather than duplicating it in your own css. In this example I would make the following changes to the html:

<ul id="accordion">
  <li><div id="profile"><i class="fa fa-user"></i>Profile</div></li>
  <li><div id="messages"><i class="fa fa-envelope"></i>Messages</div>
    <ul class="submenu">
      <li><i class="fa fa-caret-right"></i>Inbox</li>
      <li><i class="fa fa-caret-right"></i>Sent</li>
      <li><i class="fa fa-caret-right"></i>Drafts</li>
    </ul>  
  </li>
  <li><div id="settings"><i class="fa fa-cog"></i>Settings</div></li>
  <li><div id="logout"><i class="fa fa-sign-out"></i>Logout</div></li>
</ul>

See this forked fiddle. Note that I had to fix your reference to the fontawesome css, which was giving me a 404 error, and I also deleted the bit of your css that was doing all the ":before" classes.

As a side note, I totally agree with @kleinfreund on almost all his points.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CodeReview. Nice answer but... please take a moment and copy the relevant code from your forked fiddle back in to this answer as that is a more permanent way to link the suggested changes with the question (and does not rely on an external site to keep your answer valid). \$\endgroup\$ – rolfl Jan 29 '14 at 13:03
  • \$\begingroup\$ @rolfl, Well, I think the relevant code IS already in the answer. This is the only code you need to make it work, with the exception of the removal of some code in the css, which I outlined. Should I copy all the css just to remove the bad parts (which are buried at the bottom?) \$\endgroup\$ – Nathan Jan 29 '14 at 15:32
  • \$\begingroup\$ you're the expert in this case. If you think the answer stands on it's own without the need for the fiddle (the fiddle is just a nice-to-have), then I'm satisfied. \$\endgroup\$ – rolfl Jan 29 '14 at 15:36
  • \$\begingroup\$ Thank you for the answer @ Nathan. My solution lies in both yours as well as @kleinfreund's answer. Which has put me in dilemma as to which one to accept :-| \$\endgroup\$ – md1hunox Jan 29 '14 at 17:55
  • 1
    \$\begingroup\$ @vineetrok It doesn't need to be the one with the highest votes. Whatever answer is the best in your opinion should be accepted. \$\endgroup\$ – kleinfreund Jan 29 '14 at 19:09
2
\$\begingroup\$

Since you have already other answers , I will focus on different issues

This :

#accordion > li:first-child >div{
  border-radius: 5px 5px 0 0;
}

#accordion > li:last-child >div{
  border-radius: 0 0 5px 5px;
}

Is not needed. You have already border-radius on the container.

And the same for that:

#accordion > li > div{
  background-color:#383C48;

Not a big issue, but the duplicity of styles makes your life harder later. (When you are changing the border-radius and see that it doesn't change anything)

\$\endgroup\$
  • \$\begingroup\$ I'd like to point out that, removing the border radius from first and last child has only one minor difference.ie. the subtle inset box-shadow which is at the top around the corners is lost. Not a big deal, but noticeable only if you look closely. :) \$\endgroup\$ – md1hunox Jan 30 '14 at 7:03

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.