Issue 4925 - Create accordion component for Help Center

static/js/main.js

Index: static/js/main.js
===================================================================
--- a/static/js/main.js
+++ b/static/js/main.js
@@ -44,10 +44,113 @@
     {
       customSelectEls[i]
         .addEventListener("click", onClickCustomSelect, false);
       customSelectEls[i]
         .nextElementSibling
         .setAttribute("aria-hidden", "true");
     }
 
+
+    // Accordion menu

[juliandoucette, 2017/09/20 14:19:38]

NIT: I prefer an OO approach (an Accordion Function)

[ire, 2017/09/25 11:25:18]

Done.

+
+    function setupAccordionMenu(accordionEl)

[juliandoucette, 2017/09/20 14:19:37]

Suggest: "accordion"

"No hungarian notation, no special variable name prefixes or suffixes denoting
type or scope. All variable names start with a lower case letter."

(The same applies elsewhere.)

[ire, 2017/09/25 11:25:18]

Done. Does this apply to the "accordionButtons"? Since their actual name is
`accordion-toggle-button`

[juliandoucette, 2017/09/26 18:57:05]

I don't think so. But it's hard to explain why :D - I'll ask someone.

[ire, 2017/09/28 14:48:52]

Haha okay. I don't think it applies either, but I can't explain either :P

+    {
+      var accordionButtons = accordionEl.getElementsByClassName('accordion-toggle-button');
+      
+      for ( var i = 0; i < accordionButtons.length; i++ )

[juliandoucette, 2017/09/20 14:19:38]

NIT: I think that we don't put spaces inside these braces

[ire, 2017/09/25 11:25:17]

Done.

+      {
+        accordionButtons[i]

[juliandoucette, 2017/09/20 14:19:38]

Suggest: Initialize state before adding event listeners (I couldn't come up with
a valid reason for this besides personal preference. You may disagree.)

[ire, 2017/09/25 11:25:17]

I don't have an opinion either way. Done.

[juliandoucette, 2017/09/26 18:57:06]

You didn't do this?

[ire, 2017/09/28 14:48:52]

Oops, missed this. Done now (I hope :p)

+          .addEventListener("click", toggleAccordionSection, false);
+        accordionButtons[i]
+          .addEventListener("keydown", navigateAccordionHeadings, false);
+
+        // Close all sections besides the first

[juliandoucette, 2017/09/20 14:19:37]

Suggest: "except" instead of "besides" :D

"besides" is *inclusive* and "except" is *exclusive*. I think that you mean to
*exclude* the first from the group of closed sections.

[ire, 2017/09/25 11:25:18]

I've been using "besides" wrong all my life :O haha

+        if ( i !== 0 )
+        {
+          accordionButtons[i].setAttribute("aria-expanded", "false");
+          accordionButtons[i].setAttribute("aria-disabled", "false");

[juliandoucette, 2017/09/20 14:19:38]

I think that aria-disabled is only appropriate when one is not able to enable
(expand) a section (e.g. when JavaScript is disabled or if one accordion section
must be enabled at all times - which is not currently the case).

(The same applies elsewhere.)

[ire, 2017/09/25 11:25:17]

Yup, see my comment above.

+          document
+            .getElementById( accordionButtons[i].getAttribute("aria-controls") )
+            .setAttribute("hidden", "true");
+        }
+      }
+    }
+    
+    function toggleAccordionSection()

[juliandoucette, 2017/09/20 14:19:38]

NIT: This function name sounds functional, but it's used as an event handler
with a specific context e.g. you would have to call
toggleAccordionSection.call(parentParentOfAccordion) to use it otherwise. I
would suggest using a more context specific name e.g. handleAccordionClick or
onAccordionClick and perhaps calling a functional toggleAccordion(accordion)
within.

[ire, 2017/09/25 11:25:17]

I generally prefer explanatory names, particularly in cases where all this
function does is what is explained by the name. 

(Also, as I changed this to an OO approach, there is no option of the function
being called by something else anymore).

[juliandoucette, 2017/09/26 18:57:05]

I agree. But I think that the context is misleading. It would be more accurate
to call this function toggleThisAccordionButtonsSection... if you know what I
mean?
You can still

var accordion = new Accordion(accordion);
accordion.toggleAccordionSelection();

using this pattern.

You could make this function private by defining it within a closure or
prefacing it with "_" in the Object.prototype ala [In classes, prefix private
functions with a single underscore to make them
pseudo-private.](https://adblockplus.org/coding-style#javascript)

---

Anyway, since you have decided to define an "Accordian" function (class), why
not define a callable "toggle(section)" function (method) and then just bind it
correctly in the click handler? e.g.

accordionButtons[i].addEventListener("click", this.toggle.bind(this,
[accordionButtons[i]]), false);

OR

accordion.addEventListener("click", function(event)
{
  if (event.currentTarget.classList.has("accordion-toggle-button"))
this.toggle(event.currentTarget);
}.bind(this), false);

Detail: The reason that we usually suggest adding event listeners to the parent
in cases like these is because their could theoretically be infinite sections in
a component like this and each event listener (and loop iteration) costs time
and money (cpu and memory).

[ire, 2017/09/28 14:48:52]

Yes, I created the issue #5652 to refactor this and others, but I'll just handle
this here. I changed both event listeners

+    {
+      // Hide currently expanded section
+      var accordion = this.parentElement.parentElement;
+      var expandedButton = accordion.querySelector("button[aria-expanded='true']");
+      if ( expandedButton )

[juliandoucette, 2017/09/20 14:19:38]

NIT/Suggest: Make it clearer that you are checking if there is currently a
section expanded.

[ire, 2017/09/25 11:25:18]

Doesn't the name "expandedButton" do that?

[juliandoucette, 2017/09/26 18:57:05]

Yes? :/ (I have no idea what I was thinking :D)

+      {
+        expandedButton.setAttribute("aria-expanded", "false");
+        expandedButton.setAttribute("aria-disabled", "false");
+        document
+          .getElementById( expandedButton.getAttribute("aria-controls") )
+          .setAttribute("hidden", "true");
+      }
+
+      if ( expandedButton === this ) return;

[juliandoucette, 2017/09/20 14:19:37]

NIT/Suggest: Make it clearer that you are handling the case where you have
clicked to contract the currently expanded section.

[ire, 2017/09/25 11:25:18]

Done.

+
+      // Expand new section
+      this.setAttribute("aria-expanded", "true");
+      this.setAttribute("aria-disabled", "true");
+      document
+        .getElementById( this.getAttribute("aria-controls") )
+        .removeAttribute("hidden");

[juliandoucette, 2017/09/20 14:19:38]

NIT/Suggest: .focus() too? So that you could click and then arrow or tab
forward/backward?

[ire, 2017/09/25 11:25:17]

Why focus an element that isn't typically focusable (a `<dd>` element)? I've
already added event listeners on the arrow keys so a user can arrow
forward/backward.

[juliandoucette, 2017/09/26 18:57:05]

I think that I was confused. What I was really suggesting was that you should be
able to click to open one section and then press up/down tab/shift tab from
there. This was probably the wrong place to leave that comment.

[ire, 2017/09/28 14:48:52]

Now I think I'm confused :/

But currently, you can click to open up a section and press up/down tab/shift
from there.

+    }
+
+    function navigateAccordionHeadings(e)

[juliandoucette, 2017/09/20 14:19:37]

NIT/Suggest: "event"

Julian Doucette: Does our coding style permit the shorthand argument name "e"
for "event"?

kzar: I might be wrong but I don't think we have a rule about that, though in
some situations "e" for event can be ambiguous with "e" for element in my
experience. Also when you use names like "e" you're more likely to violate the
no-shadow rule. Personally I try to go for the full name where possible, but
sometimes find that doesn't make sense.

[ire, 2017/09/25 11:25:17]

Done.

+    {
+      if ( e.keyCode !== 38 && e.keyCode !== 40 ) return;

[juliandoucette, 2017/09/20 14:19:38]

NIT/Suggest: Prefer event.key and fall back to event.keyCode

[ire, 2017/09/25 11:25:17]

Done.

+
+      var accordion = this.parentElement.parentElement;
+
+      // Direction == up
+      if ( e.keyCode == 38 )

[juliandoucette, 2017/09/20 14:19:38]

NIT/Suggest: Name keycodes e.g. var KEY_UP = 38.

[ire, 2017/09/25 11:25:18]

Done.

+      {
+        var prevEl = this.parentElement.previousElementSibling;
+        if ( prevEl )

[juliandoucette, 2017/09/20 14:19:38]

NIT/Suggest: Make it clearer that you are checking if you are at the first or
last section. e.g. isLast, isNotLast, isFirst, isNotFirst

[ire, 2017/09/25 11:25:18]

This would mean I cannot use the same variable name below, e.g.:

var isFirst = this.parentElement.previousElementSibling;
if ( isFirst )
  isFirst.previousElementSibling.firstElementChild.focus();

I would have to query for the previous element again because the variable name
isFirst doesn't make sense there.

[juliandoucette, 2017/09/26 18:57:05]

Acknowledged.

+        {
+          prevEl // .accordion-body
+            .previousElementSibling // .accordion-heading
+            .firstElementChild // .accordion-toggle-button
+            .focus();
+        }
+        else
+        {
+          accordion
+            .lastElementChild // .accordion-body
+            .previousElementSibling // .accordion-heading
+            .firstElementChild // .accordion-toggle-button
+            .focus();
+        }
+      }
+
+      // Direction == down
+      else if ( e.keyCode == 40 )
+      {
+        var nextheading = this.parentElement.nextElementSibling.nextElementSibling;
+        if ( nextheading )
+        {
+          nextheading // .accordion-heading
+            .firstElementChild // .accordion-toggle-button
+            .focus();
+        }
+        else
+        {
+          accordion
+            .firstElementChild // .accordion-heading
+            .firstElementChild // .accordion-toggle-button
+            .focus();
+        }
+      }
+    }
+
+    var accordionMenus = document.getElementsByClassName('accordion');
+    for (var i = 0; i < accordionMenus.length; i++)
+    {
+      setupAccordionMenu( accordionMenus[i] );
+    }
+
   }, false);
 }());