feat(accordions): provide accordions component - INNO-615 #207
Conversation
| } | ||
|
|
||
| .ecl-accordions__panel { | ||
| background-color: #f5f5f5; |
There was a problem hiding this comment.
should use ecl-colors palette (grey-5)
There was a problem hiding this comment.
My bad, thanks for noticing
| * @define accordions | ||
| */ | ||
|
|
||
| .ecl-accordions__header { |
There was a problem hiding this comment.
should use "accordion" instead of "accordions" for classes, as this is what we do for other components.
There was a problem hiding this comment.
Could be. I'll rewrite it ;)
| }; | ||
|
|
||
| // module exports | ||
| export default accordions; |
There was a problem hiding this comment.
so in the end, accordions do not rely on expandables at all. That's quite a shame, but that's probably easier this way.
There was a problem hiding this comment.
I'm thinking about making the "expandable" JS part of ecl-base. Kind of a helper which we would reuse in accordion, tabs, megamenu...
There was a problem hiding this comment.
Created INNO-640 for following-up
| @@ -0,0 +1,18 @@ | |||
| <div class="ecl-accordions" role="tablist" aria-multiselectable="true"> | |||
| <h2 id="accordion-header-1" class="ecl-accordions__header" role="tab" aria-controls="accordion-panel-1" tabindex="0" aria-selected="true" aria-expanded="true"> | |||
There was a problem hiding this comment.
I don't like using headings inside components, as we don't know if they will match page hierarchy correctly.
Moreover, as you don't use specific ecl classes for headings, it make no real sense.
| <h2 id="accordion-header-1" class="ecl-accordions__header" role="tab" aria-controls="accordion-panel-1" tabindex="0" aria-selected="true" aria-expanded="true"> | ||
| <span class="ecl-accordions__header-icon ecl-icon ecl-icon--rounded ecl-u-bg-secondary ecl-icon--growth"></span> Jobs, Growth, Investment and Competitiveness with an additional quite long string | ||
| </h2> | ||
| <div id="accordion-panel-1" class="ecl-accordions__panel" role="tabpanel" aria-labelledby="accordion-header-1" tabindex="0" aria-hidden="false"> |
There was a problem hiding this comment.
What is the reason to set a tabindex here? It somehow mess with keyboard navigation (when using tabs)
There was a problem hiding this comment.
Good remark, I'll remove it. http://www.oaa-accessibility.org/examplep/accordian1/ doesn't declare it either
| "license": "EUPL-1.1", | ||
| "version": "0.1.0", | ||
| "description": "ECL Accordions", | ||
| "main": "_ecl-accordions.scss", |
There was a problem hiding this comment.
in other components we didn't put "ecl-" as prefix for sass file. I don't really know the reason, but we should keep it consistant.
|
Another small remark: on small screens icons and margin take a lots of space. Maybe we could reduce the margin here. |
There was a problem hiding this comment.
The solution works well for me.
I couldn't test the arrow navigation in Safari because the tabbing there is not easy, but IE9 worked, so I trust Safar should also...
Test that you can navigate with the keyboard (arrows up/down, space)