HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Minimalist accordion implementation using vanilla JavaScript

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptaccordionvanillausingimplementationminimalist

Problem

The following example is an accordion script in JavaScript that does not use jQuery. When I started looking for a simple solution to this requirement I did not accept the idea of having to use a powerful and robust library for a simple implementation. I didn't like the idea of use a machine gun when what I really need is a slingshot. So as a jQuery detox process I decide create my own solution. So far the code is simple enough and I’m planning using it in production as a custom widget in my blog running Wordpress. I’m very enthusiast of minimalist development and I have some concerns with this code.

Any suggestions about how I can achieve a simpler and elegant solution? I was thinking in implementing the element.classList properties.

Here is a live demo of the accordion script.

HTML

Archivos


  • 2015





  • febrero 2015



  • enero 2015





  • 2014





  • diciembre 2014



  • noviembre 2014



  • marzo 2014



  • febrero 2014



  • enero 2014





  • 2013





  • diciembre 2013



  • septiembre 2013








CSS

`.widget
{
width:60%;
margin:50px auto 0 auto;
border:4px solid rgba(10,10,10,0.05);
padding:10px;
border-radius:10px;
-webkit-box-shadow: 10px 10px 45px -20px rgba(0,0,0,0.75);
-moz-box-shadow: 10px 10px 45px -20px rgba(0,0,0,0.75);
box-shadow: 10px 10px 45px -20px rgba(0,0,0,0.75);
}

.widget-title
{
font-weight:bold;
text-transform:uppercase;
color:#636363;
padding-bottom:10px
}

.widget a
{
color:#9b9b9b;
text-decoration:none;
padding:10px 0 10px 10px;
display:inline-block;
}

.widget a + a
{
padding:10px 16px;
float:right;
line-height:1.5;
}

.widget a + a:hover
{
background-color:#ddd;
color:#fff;
}

.widget a:hover
{
color:#444
}

.widget ul
{
list-style-type:none;
margin:0;
padding:0;
background-color:#eee;
}

.widget ul
{
line-height:1.5;
}

.widget li
{
border-top

Solution

Yes, you should use classList. Right now you overwrite the entire className attribute of the elements. It works, but it also means that you can never use any other class name on the elements. So it's just not a very precise or robust way of doing things; it only works as long as you don't add other classes. And if you do need to add another class to the elements at some point, your script will mess up because the comparison with "" will always fail, and then any and all class names will get summarily removed by your script.

Of course, classList isn't supported everywhere, so you may have to use a shim/polyfill. The nextElementSibling property isn't supported everywhere either, by the way (which, incidentally is why jQuery exists; because browser compatibility is not simple).

I also wonder why you use querySelector in one line, but elementsByClassName in the next line. In both cases you want to find element(s) with a given class name. Of course, in the first case, you want exactly 1 element, and in the other you want a collection. However, if you want 1 particular element, you should probably add an id attribute to that element - not a class - and then use the good old document.getElementById. In other words: Using a class selector and expecting it to only exist on one element is backwards. Classes are meant to be used on an arbitrary number of elements. If you want unique identification, use an id instead.

Also, please don't mix languages in your code. I know it's only two variable names, but especially because it's only two variable names there's no reason to switch language. The DOM API is in English, and even all your CSS classes are in English, so it's simpler and cleaner to just stick to that (by the way, I'm not a native English speaker myself, I just prefer code to be consistent.)

Lastly, I always recommend that you use brace-on-same-line style in JavaScript. There are some assumptions about style in JS interpreters, and brace-on-same-line is one.

Context

StackExchange Code Review Q#83940, answer score: 2

Revisions (0)

No revisions yet.