Issue 29548567: Issue 4925 - Create accordion component for Help Center (Closed)
|
Created:
Sept. 18, 2017, 10:03 a.m. by ire Modified:
Oct. 10, 2017, 6:39 p.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/help.eyeo.com Visibility:
Public. |
DescriptionIssue 4925 - Create accordion component for Help Center
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add support for navigating between headings with arrow keys #Patch Set 3 : Rebase #
Total comments: 40
Patch Set 4 : Rebase, Accordion object, hover state #
Total comments: 6
Patch Set 5 : Refactoring #
Total comments: 1
MessagesTotal messages: 16
Ready for review. In this implementation I did not add any relevant content. Just placeholders. I think it will be best to add the content in a later patchset, which will be on top of a patchset that has the articles (e.g. when I push the Product Help Home issue). I also just placed the accordion menu on the homepage. This will also be changed and moved to the article page when that is ready. https://codereview.adblockplus.org/29548567/diff/29548568/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29548568/static/js/main.js#n... static/js/main.js:38: .setAttribute("hidden", "true"); See https://issues.adblockplus.org/ticket/5703 regarding use of the hidden attribute https://codereview.adblockplus.org/29548567/diff/29548568/static/js/main.js#n... static/js/main.js:66: } This will be refactored in https://issues.adblockplus.org/ticket/5652 https://codereview.adblockplus.org/29548567/diff/29548568/static/scss/compone... File static/scss/components/_accordion.scss (right): https://codereview.adblockplus.org/29548567/diff/29548568/static/scss/compone... static/scss/components/_accordion.scss:55: transform: rotate(180deg); transforms aren't supported by IE8 (http://caniuse.com/#feat=transforms2d) . AFAIK it's still undecided if IE8 is supported by the Help Center
Looks good at first glance. I have a few questions before I dig deeper though: - Why didn't you support arrow keys? - Why didn't you use cursor:pointer? - Why didn't you focus dt on click so that you can tab or arrow to the next item from click? - Did you base this on https://www.w3.org/TR/wai-aria-practices/examples/accordion/accordion.html or does it just bare a striking resemblance :P (I recommend basing it on that)? https://codereview.adblockplus.org/29548567/diff/29548568/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29548568/static/js/main.js#n... static/js/main.js:38: .setAttribute("hidden", "true"); On 2017/09/18 10:08:11, ire wrote: > See https://issues.adblockplus.org/ticket/5703 regarding use of the hidden > attribute Acknowledged. https://codereview.adblockplus.org/29548567/diff/29548568/static/js/main.js#n... static/js/main.js:66: } On 2017/09/18 10:08:11, ire wrote: > This will be refactored in https://issues.adblockplus.org/ticket/5652 Acknowledged.
On 2017/09/18 12:53:31, juliandoucette wrote: > Looks good at first glance. I have a few questions before I dig deeper though: Thanks :) > - Did you base this on > https://www.w3.org/TR/wai-aria-practices/examples/accordion/accordion.html or > does it just bare a striking resemblance :P (I recommend basing it on that)? I actually based this on https://www.w3.org/TR/2016/WD-wai-aria-practices-1.1-20161214/examples/accord... , which appears to be a more outdated version of the example you demonstrated. > - Why didn't you support arrow keys? The (outdated) example I followed didn't have arrow key support > - Why didn't you focus dt on click so that you can tab or arrow to the next item > from click? Same answer as above > - Why didn't you use cursor:pointer? An oversight, will add in the next patch. --- Based on me using the outdated example, would you prefer I updated my implementation to support arrow keys before or after you have a look at the current implementation?
On 2017/09/18 14:57:02, ire wrote: > Based on me using the outdated example, would you prefer I updated my > implementation to support arrow keys before or after you have a look at the > current implementation? Before; please :)
On 2017/09/18 15:49:04, juliandoucette wrote: > On 2017/09/18 14:57:02, ire wrote: > > Based on me using the outdated example, would you prefer I updated my > > implementation to support arrow keys before or after you have a look at the > > current implementation? > > Before; please :) Done :) > Why didn't you focus dt on click so that you can tab or arrow to the next item from click? I couldn't find any documentation on doing it this way (focusing the dt), so my implementation doesn't use this method.
This Patchset doesn't seem to apply?
On 2017/09/19 14:11:26, juliandoucette wrote: > This Patchset doesn't seem to apply? Rebase. Sorry about that!
Note: The only non-NIT / non-suggestion is the aria-disabled issue. https://codereview.adblockplus.org/29548567/diff/29549875/pages/index.md File pages/index.md (right): https://codereview.adblockplus.org/29548567/diff/29549875/pages/index.md#newc... pages/index.md:22: <dt role="heading" aria-level="3" class="accordion-heading"> I think that the default (non-js) state is aria-expanded and aria-disabled. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:53: // Accordion menu NIT: I prefer an OO approach (an Accordion Function) https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:55: function setupAccordionMenu(accordionEl) 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.) https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:59: for ( var i = 0; i < accordionButtons.length; i++ ) NIT: I think that we don't put spaces inside these braces https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:61: accordionButtons[i] Suggest: Initialize state before adding event listeners (I couldn't come up with a valid reason for this besides personal preference. You may disagree.) https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:66: // Close all sections besides the first 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. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:70: accordionButtons[i].setAttribute("aria-disabled", "false"); 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.) https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:78: function toggleAccordionSection() 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. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:83: if ( expandedButton ) NIT/Suggest: Make it clearer that you are checking if there is currently a section expanded. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:92: if ( expandedButton === this ) return; NIT/Suggest: Make it clearer that you are handling the case where you have clicked to contract the currently expanded section. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:99: .removeAttribute("hidden"); NIT/Suggest: .focus() too? So that you could click and then arrow or tab forward/backward? https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:102: function navigateAccordionHeadings(e) 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. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:104: if ( e.keyCode !== 38 && e.keyCode !== 40 ) return; NIT/Suggest: Prefer event.key and fall back to event.keyCode https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:109: if ( e.keyCode == 38 ) NIT/Suggest: Name keycodes e.g. var KEY_UP = 38. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:112: if ( prevEl ) NIT/Suggest: Make it clearer that you are checking if you are at the first or last section. e.g. isLast, isNotLast, isFirst, isNotFirst
One more thing before I forget: I suggest that we add a more distinct hover/focus/active state (or create an issue requesting this change).
Thanks! > One more thing before I forget: I suggest that we add a more distinct > hover/focus/active state (or create an issue requesting this change). Done. https://codereview.adblockplus.org/29548567/diff/29549875/pages/index.md File pages/index.md (right): https://codereview.adblockplus.org/29548567/diff/29549875/pages/index.md#newc... pages/index.md:22: <dt role="heading" aria-level="3" class="accordion-heading"> On 2017/09/20 14:19:37, juliandoucette wrote: > I think that the default (non-js) state is aria-expanded and aria-disabled. Yup you're right. My mistake. Also, I realised I don't need aria-disabled anymore because I allowed an expanded section to be closed. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:53: // Accordion menu On 2017/09/20 14:19:38, juliandoucette wrote: > NIT: I prefer an OO approach (an Accordion Function) Done. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:55: function setupAccordionMenu(accordionEl) On 2017/09/20 14:19:37, juliandoucette wrote: > 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.) Done. Does this apply to the "accordionButtons"? Since their actual name is `accordion-toggle-button` https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:59: for ( var i = 0; i < accordionButtons.length; i++ ) On 2017/09/20 14:19:38, juliandoucette wrote: > NIT: I think that we don't put spaces inside these braces Done. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:61: accordionButtons[i] On 2017/09/20 14:19:38, juliandoucette wrote: > Suggest: Initialize state before adding event listeners (I couldn't come up with > a valid reason for this besides personal preference. You may disagree.) I don't have an opinion either way. Done. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:66: // Close all sections besides the first On 2017/09/20 14:19:37, juliandoucette wrote: > 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. I've been using "besides" wrong all my life :O haha https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:70: accordionButtons[i].setAttribute("aria-disabled", "false"); On 2017/09/20 14:19:38, juliandoucette wrote: > 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.) Yup, see my comment above. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:78: function toggleAccordionSection() On 2017/09/20 14:19:38, juliandoucette wrote: > 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. 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). https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:83: if ( expandedButton ) On 2017/09/20 14:19:38, juliandoucette wrote: > NIT/Suggest: Make it clearer that you are checking if there is currently a > section expanded. Doesn't the name "expandedButton" do that? https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:92: if ( expandedButton === this ) return; On 2017/09/20 14:19:37, juliandoucette wrote: > NIT/Suggest: Make it clearer that you are handling the case where you have > clicked to contract the currently expanded section. Done. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:99: .removeAttribute("hidden"); On 2017/09/20 14:19:38, juliandoucette wrote: > NIT/Suggest: .focus() too? So that you could click and then arrow or tab > forward/backward? 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. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:102: function navigateAccordionHeadings(e) On 2017/09/20 14:19:37, juliandoucette wrote: > 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. Done. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:104: if ( e.keyCode !== 38 && e.keyCode !== 40 ) return; On 2017/09/20 14:19:38, juliandoucette wrote: > NIT/Suggest: Prefer event.key and fall back to event.keyCode Done. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:109: if ( e.keyCode == 38 ) On 2017/09/20 14:19:38, juliandoucette wrote: > NIT/Suggest: Name keycodes e.g. var KEY_UP = 38. Done. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:112: if ( prevEl ) On 2017/09/20 14:19:38, juliandoucette wrote: > NIT/Suggest: Make it clearer that you are checking if you are at the first or > last section. e.g. isLast, isNotLast, isFirst, isNotFirst 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.
https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:55: function setupAccordionMenu(accordionEl) On 2017/09/25 11:25:18, ire wrote: > On 2017/09/20 14:19:37, juliandoucette wrote: > > 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.) > > Done. Does this apply to the "accordionButtons"? Since their actual name is > `accordion-toggle-button` I don't think so. But it's hard to explain why :D - I'll ask someone. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:61: accordionButtons[i] On 2017/09/25 11:25:17, ire wrote: > On 2017/09/20 14:19:38, juliandoucette wrote: > > Suggest: Initialize state before adding event listeners (I couldn't come up > with > > a valid reason for this besides personal preference. You may disagree.) > > I don't have an opinion either way. Done. You didn't do this? https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:78: function toggleAccordionSection() On 2017/09/25 11:25:17, ire wrote: > I generally prefer explanatory names, particularly in cases where all this > function does is what is explained by the name. 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? > (Also, as I changed this to an OO approach, there is no option of the function > being called by something else anymore). 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). https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:83: if ( expandedButton ) On 2017/09/25 11:25:18, ire wrote: > On 2017/09/20 14:19:38, juliandoucette wrote: > > NIT/Suggest: Make it clearer that you are checking if there is currently a > > section expanded. > > Doesn't the name "expandedButton" do that? Yes? :/ (I have no idea what I was thinking :D) https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:99: .removeAttribute("hidden"); On 2017/09/25 11:25:17, ire wrote: > 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. 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. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:112: if ( prevEl ) On 2017/09/25 11:25:18, ire wrote: > On 2017/09/20 14:19:38, juliandoucette wrote: > > NIT/Suggest: Make it clearer that you are checking if you are at the first or > > last section. e.g. isLast, isNotLast, isFirst, isNotFirst > > 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. Acknowledged. https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js#n... static/js/main.js:57: this.init(); NIT: It's unnecessary to separate this constructor logic if we are not planning to override it later (at which point we could separate it). https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js#n... static/js/main.js:105: Accordion.prototype.navigateAccordionHeadings = function(event) Suggest: Accordion.prototype._onKeyDown(event) { if (event.key === "ArrowUp" || event.keyCode === 38) this.toggleNextSection(); if (event.key === "ArrowDown" || event.keyCode === 40) this.togglePrevSection(); } Accordion.prototype.toggleNextSection() { var toggledSectionBody = this.accordion.querySelector('[aria-expanded="true"]').parentElement.nextElementSibling; if (this.accordion.children[this.accordion.children.length - 1] === toggledSectionBody) // if the last section is already expanded ... } Accordion.prototype.togglePrevSection() { var toggledSectionHeading = this.accordion.querySelector('[aria-expanded="true"]').parentElement; if (this.accordion.children[0] === toggledSectionHeading) // if the first section is already expanded ... } ... (Or similar - I'm just thinking out loud about how this code could be made clearer.) https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js#n... static/js/main.js:107: var IS_KEY_UP = event.key == 38 || event.keyCode == 38; I think that the value of event.key for up is "ArrowUp" and down is "ArrowDown".
Updated. https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:55: function setupAccordionMenu(accordionEl) On 2017/09/26 18:57:05, juliandoucette wrote: > On 2017/09/25 11:25:18, ire wrote: > > On 2017/09/20 14:19:37, juliandoucette wrote: > > > 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.) > > > > Done. Does this apply to the "accordionButtons"? Since their actual name is > > `accordion-toggle-button` > > I don't think so. But it's hard to explain why :D - I'll ask someone. Haha okay. I don't think it applies either, but I can't explain either :P https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:61: accordionButtons[i] On 2017/09/26 18:57:06, juliandoucette wrote: > On 2017/09/25 11:25:17, ire wrote: > > On 2017/09/20 14:19:38, juliandoucette wrote: > > > Suggest: Initialize state before adding event listeners (I couldn't come up > > with > > > a valid reason for this besides personal preference. You may disagree.) > > > > I don't have an opinion either way. Done. > > You didn't do this? Oops, missed this. Done now (I hope :p) https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:78: function toggleAccordionSection() On 2017/09/26 18:57:05, juliandoucette wrote: > On 2017/09/25 11:25:17, ire wrote: > > I generally prefer explanatory names, particularly in cases where all this > > function does is what is explained by the name. > > 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? > > > (Also, as I changed this to an OO approach, there is no option of the function > > being called by something else anymore). > > 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). Yes, I created the issue #5652 to refactor this and others, but I'll just handle this here. I changed both event listeners https://codereview.adblockplus.org/29548567/diff/29549875/static/js/main.js#n... static/js/main.js:99: .removeAttribute("hidden"); On 2017/09/26 18:57:05, juliandoucette wrote: > On 2017/09/25 11:25:17, ire wrote: > > 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. > > 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. Now I think I'm confused :/ But currently, you can click to open up a section and press up/down tab/shift from there. https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js#n... static/js/main.js:57: this.init(); On 2017/09/26 18:57:06, juliandoucette wrote: > NIT: It's unnecessary to separate this constructor logic if we are not planning > to override it later (at which point we could separate it). Done. https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js#n... static/js/main.js:105: Accordion.prototype.navigateAccordionHeadings = function(event) On 2017/09/26 18:57:06, juliandoucette wrote: > Suggest: > > Accordion.prototype._onKeyDown(event) > { > if (event.key === "ArrowUp" || event.keyCode === 38) this.toggleNextSection(); > if (event.key === "ArrowDown" || event.keyCode === 40) > this.togglePrevSection(); > } > > Accordion.prototype.toggleNextSection() > { > var toggledSectionBody = > this.accordion.querySelector('[aria-expanded="true"]').parentElement.nextElementSibling; > > if (this.accordion.children[this.accordion.children.length - 1] === > toggledSectionBody) // if the last section is already expanded > ... > } > > Accordion.prototype.togglePrevSection() > { > var toggledSectionHeading = > this.accordion.querySelector('[aria-expanded="true"]').parentElement; > > if (this.accordion.children[0] === toggledSectionHeading) // if the first > section is already expanded > ... > } > ... > > (Or similar - I'm just thinking out loud about how this code could be made > clearer.) Made some changes. https://codereview.adblockplus.org/29548567/diff/29555703/static/js/main.js#n... static/js/main.js:107: var IS_KEY_UP = event.key == 38 || event.keyCode == 38; On 2017/09/26 18:57:06, juliandoucette wrote: > I think that the value of event.key for up is "ArrowUp" and down is "ArrowDown". Done. (Sorry I didn't check :/)
LGTM - But I was unable to test because the patch does not apply. Please rebase and test before pushing. https://codereview.adblockplus.org/29548567/diff/29558626/static/js/main.js File static/js/main.js (right): https://codereview.adblockplus.org/29548567/diff/29558626/static/js/main.js#n... static/js/main.js:66: .getElementById( accordionButtons[i].getAttribute("aria-controls") ) NIT: Extra whitespace inside brackets (The same applies elsewhere)
Scratch that - I was able to get this to work on -r 7. Still LGTM - But I think that the dark grey hover/active/focus state is way too dark :/ ?
On 2017/10/09 21:24:28, juliandoucette wrote: > Scratch that - I was able to get this to work on -r 7. Still LGTM - But I think > that the dark grey hover/active/focus state is way too dark :/ ? I created an issue for this https://issues.adblockplus.org/ticket/5852#ticket 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
