2
\$\begingroup\$

Accordion plugin

As I don't use jQuery (because I just have fun with Chrome and iOS/Android) and I prefer to write my own functions, I decided to write a simple Accordion function some time ago:

  • Short code
  • High performance
  • Latest JS CSS3 HTML5 technologies

I don't want to add many classes and ids, and many eventhandlers. (My iPad 1 is very sensitive with too many eventhandlers and they are also a pain to delete if needed)

This function adds no ids, no extra classes and uses only one eventhandler.

Single Accordion:

Code for single Accordions (you can apply this function to more elements but you can't put accordions inside accordions.):

function acsf(e) {
    var Accordion = e.target.parentNode;
    if (Accordion.parentNode == this && Accordion.firstChild == e.target) {
        if (this.CurrentAccordion == Accordion) {
            this.CurrentAccordion.style.height = '';
            delete this.CurrentAccordion
        } else {
            !this.CurrentAccordion || (this.CurrentAccordion.style.height = '');
            this.CurrentAccordion = Accordion;
            this.CurrentAccordion.style.height =
                (Accordion.firstChild.offsetHeight +
                this.CurrentAccordion.childNodes[1].offsetHeight) + 'px'
        }
    }
}

Structure (no new lines or spaces between <>):

<div class="accordion">
    <div>
        <div>Title</div>
        <div>Content</div>
    </div>
    <div>
        <div>Title</div>
        <div>Content</div>
    </div>
</div>

Usage:

document.getElementsByClassName('accordion')[0].addEventListener('click',acsf,false);

The above code works perfectly.

http://jsfiddle.net/zhD5L/

But I want to create a plugin that adds this function to every element with the accordion class and include it as a script:

(function() {
 function acsf() {
  //the code above
 }
 function init(){
  var acs = D.getElementsByClassName('accordion'),
  acsl = acs.length;
  while(acsl--){
      acs[acsl].addEventListener('click',acsf,false);
  }
  window.removeEventListener('load',init,false);
 }
 window.addEventListener('load',init,false);
})()

Is this correct, or are the better ways to do that?

Should this remove the possibility of creating problems with other variable or function names?


I also want to be able to put accordions inside accordions, so I wrote this next script, but there is a delay and I totally don't know how to remove that.

I also don't want to use settimeout. That was just the only way I knew to get it work vs webkitAnimationEnd which just needs a little more code. But I need to get the next height before the animation or timeout ends.

(function (D) {
    function acsf(e) {
        var a = e.target.parentNode,
            h;
        if (a.parentNode == this && a.firstChild == e.target) {
            h = a.firstChild.offsetHeight;
            !this.c || (this.c.style.height = h + 'px');
            this.c != a ?
                (this.c = a, this.c.style.height = (h + this.c.childNodes[1].offsetHeight) + 'px') :
                (this.c.style.height = h + 'px', delete this.c);
            setTimeout(acscf.bind(this), 350);
        }
    }

    function acscf(x) {
        x = this;
        if (x.parentNode && x.parentNode.parentNode &&
            x.parentNode.parentNode.parentNode &&
            x.parentNode.parentNode.parentNode.classList) {
            var y = x.parentNode.parentNode.parentNode;
            if (y.classList.contains('accordion')) {
                console.log(x.offsetHeight);
                y.c.style.height = (48 + (x.offsetHeight)) + 'px';
                setTimeout(acscf.bind(y), 350);
            }
        }
    }

    function acsinit() {
        var acs = D.getElementsByClassName('accordion'),
            acsl = acs.length;
        while (acsl--) {
            acs[acsl].addEventListener('click', acsf, false);
        }
        window.removeEventListener('load', acsinit, false);
    }
    window.addEventListener('load', acsinit, false);
})(document)

Example to play with:

http://jsfiddle.net/x8dzh/

^ the latest accordion title contains other 2 accordions


And now to make things difficult I'm trying to find a way to interact with the closed function outside of it. So basically toggle elements with another function created inside another script.


To resume the questions:

  1. Create a plugin to include that does not create problems with other scripts. (I'm totally new to JS classes)
  2. Remove the delay
  3. I want to be able to toggle the elements outside with a function outside of the plugin.

EDIT

I just found out that I could use try catch vs the long check on the parentNodes.

if(x.parentNode && 
   x.parentNode.parentNode &&
   x.parentNode.parentNode.parentNode &&
   x.parentNode.parentNode.parentNode.classList..

to

try{
 y=x.parentNode.parentNode.parentNode.classList.contains('accordion')
}catch(e){
 y=false
}

But what about performance?

\$\endgroup\$
  • \$\begingroup\$ What happened to the indentation? You really use 1 space and no whitespace? It reads like minified code. \$\endgroup\$ – elclanrs Jan 21 '14 at 19:39
  • \$\begingroup\$ i personally use only 1 space and no white spaces .. but in this case i also use it so there is no need to scroll. \$\endgroup\$ – cocco Jan 21 '14 at 19:41
  • \$\begingroup\$ this is how i wrote it first jsfiddle.net/zhD5L/1 i use notepad. \$\endgroup\$ – cocco Jan 21 '14 at 19:43
  • 1
    \$\begingroup\$ But this is a code review site, the indentation is the very first thing I'd notice, plus the name of your variables, it doesn't help either. Many people will pass on your question after a first glance at the code. \$\endgroup\$ – elclanrs Jan 21 '14 at 19:45
  • 1
    \$\begingroup\$ I think you could just name your variables right, and use this service to indent your code jsbeautifier.org. Unless of course, you want people to comment on those things. No need to close the question. \$\endgroup\$ – elclanrs Jan 21 '14 at 19:47
3
\$\begingroup\$

Your code has a lot of style problems, so many style problems that it would take a long time to figure out what it does. Well written code does not pose this problem. I can really only point out what you are doing wrong style wise and after the code is cleaned up, perhaps we ( or you yourself ) can figure out your questions.

  • There is not a single comment
  • Accordion starts with an uppercase, that should be reserved for constructors
  • This is a shortcut for if, do not use this unless you are code golfing:

    !this.CurrentAccordion || (this.CurrentAccordion.style.height = '');
    
  • Over the top abbreviations like acsf should be avoided
  • One character variables should be avoided ( except perhaps for the common i,s and e )
  • This makes my eyes bleed, what does it do?? :

    this.c != a ?
        (this.c = a, this.c.style.height = (h + this.c.childNodes[1].offsetHeight) + 'px') :
        (this.c.style.height = h + 'px', delete this.c);
    

    using comma's to chain statements is plain wrong in a ternary

  • I have no idea why you would need to do this:

    if (x.parentNode && x.parentNode.parentNode &&
        x.parentNode.parentNode.parentNode &&
        x.parentNode.parentNode.parentNode.classList) {
    

    this is extremely brittle, at the very least I would do a recursive lookup on parentNode until I find something on the element that identifies it as the right level. Or build a function that gives the n-th level parent, but then it's brittle again. ( Meaning that a change in HTML layout will break your function ).

\$\endgroup\$
  • \$\begingroup\$ After reading what code golfing means, well yes i always do that.I want a short performant code. stackoverflow.com/a/21353032/2450730 ,codereview.stackexchange.com/q/45335/33435. i use shorthand or/& bitwise operators over standard one as they perform faster on most browsers. btw i see no examples in you answer. your just criticizing my code. actually there is no answer in your response that i could apply to my code.reguarding the last answers that checks the parentnode: brittle but performant. i already answered that with another solution with is shorter but loses in performance \$\endgroup\$ – cocco Apr 17 '14 at 9:02
  • \$\begingroup\$ jsperf.com/parseint-multihttp://jsperf.com/bw-math jsperf.com/multiple-functions jsperf.com/store-array-vs-obj/2 jsperf.com/if-short here are some performance tests. \$\endgroup\$ – cocco Apr 17 '14 at 9:04
  • \$\begingroup\$ and explain why using commas to chain is wrong pls. \$\endgroup\$ – cocco Apr 17 '14 at 9:06
  • \$\begingroup\$ Given that you know how to write code golf code, I did not think it was necessary to tell you how to write your code with if statements instead. Comma chained statements in a ternary should be handled by an if statement \$\endgroup\$ – konijn Apr 17 '14 at 13:18
  • \$\begingroup\$ Furthermore, criticizing code actually is an answer on CodeReview. What I am telling you in my review is that your code is too hard to grok, and hence too hard to fix. Write your code first in a readable manner, and then perhaps you will find the trouble. I am sorry if I hurt your feelings, but know that this code is a maintenance nightmare (you cannot fix it yourself!). \$\endgroup\$ – konijn Apr 17 '14 at 13:24

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.