feat(oui-collapsible): collapsible component addition #173
Conversation
ganeshkumar1989
requested review from
AxelPeter,
JDownloader,
cbourgois,
neolitec,
rfougier and
whispyy
|
|
||
| this.$attrs = $attrs; | ||
| this.$element = $element; | ||
| this.$timeout = $timeout; |
There was a problem hiding this comment.
this.$element and this.$timeout are not used. We can remove them.
But we still need this.$attrs for addBooleanParameter`addBooleanParameter.
There was a problem hiding this comment.
Now the component accepts an id, which is required for aria-controls attribute. This id must be removed from the original directive and hence these dependencies are now required :)
| <div class="oui-collapsible" data-ng-class="{'oui-collapsible_open':$ctrl.expanded}"> | ||
| <div class="oui-collapsible__header" data-ng-click="$ctrl.toggle()"> | ||
| <div class="oui-collapsible__header-text" data-ng-bind="::$ctrl.title"></div> | ||
| <button |
There was a problem hiding this comment.
This <button> seems useless since the click is on the parent <div>.
What do you think about replacing oui-collapsible__header by a <button> ?
<button class="oui-collapsible__header" type="button"
aria-label="{{::$ctrl.ariaLabel}}" aria-expanded="{{$ctrl.expanded}}"
ng-click="$ctrl.toggle()">
<span class="oui-collapsible__header-text" ng-bind="::$ctrl.title"></span>
<span class="oui-icon oui-icon-chevron-down oui-collapsible__toggle-icon" aria-hidden="true"></span>
</button>
There was a problem hiding this comment.
That is a very good idea :)
Expanding a panel on button click sounds good. But is having a button as a header a good idea? Please let me know your thoughts on this.
Another possibility is to remove the button and have the icon in oui-collapsible__header
There was a problem hiding this comment.
For me, it's not a problem, as long as we don't have a click on a unfocusable tag.
We could still have a wrapper around the button if necessary, but you'll need the aria-controls attribute for a11y:
eg.
<div class="oui-collapsible__header">
<button class="oui-collapsible__header-button" type="button"
aria-label="{{::$ctrl.ariaLabel}}"
aria-expanded="{{$ctrl.expanded}}"
aria-controls="collapsibleContent1"
ng-click="$ctrl.toggle()">
<span class="oui-collapsible__header-text" ng-bind="::$ctrl.title"></span>
<span class="oui-icon oui-icon-chevron-down oui-collapsible__toggle-icon" aria-hidden="true"></span>
</button>
</div>
<div class="oui-collapsible__body" id="collapsibleContent1">
[...]
</div>
There was a problem hiding this comment.
Understood. After some thought, it seems like a button for the header is a better idea. I have done this change :)
Thanks again for this idea
| @@ -0,0 +1,13 @@ | |||
| <div class="oui-collapsible" data-ng-class="{'oui-collapsible_open':$ctrl.expanded}"> | |||
| <div class="oui-collapsible__header" data-ng-click="$ctrl.toggle()"> | |||
There was a problem hiding this comment.
Accessibility issue: we have no clue when the header has the focus (tab navigation).
We could add an outline like on the navbar for this :)
There was a problem hiding this comment.
This is done :)
| @@ -0,0 +1,13 @@ | |||
| <div class="oui-collapsible" data-ng-class="{'oui-collapsible_open':$ctrl.expanded}"> | |||
There was a problem hiding this comment.
Unlike in our managers, we don't prefixing with data- in our templates
There was a problem hiding this comment.
The data- prefix has now been removed :)
| <i class="oui-icon oui-icon-chevron-down oui-collapsible__toggle-icon" aria-hidden="true"></i> | ||
| </button> | ||
| </div> | ||
| <div class="oui-collapsible__body" data-ng-transclude></div> |
There was a problem hiding this comment.
Accessibility issue: we have no aria-expanded and aria-controls.
Please see here for more informations :)
There was a problem hiding this comment.
These attrs have been added
| <button | ||
| type="button" | ||
| aria-label="{{::$ctrl.ariaLabel}}" | ||
| aria-pressed="{{$ctrl.expanded}}" |
There was a problem hiding this comment.
I think this should be aria-expanded instead
There was a problem hiding this comment.
This has been changed :)
AxelPeter
force-pushed
the
feature/oui-collapsible
branch
from
c122951 to
b7adf2d
Compare
AxelPeter
force-pushed
the
feature/oui-collapsible
branch
from
b7adf2d to
60df190
Compare
* feat(oui-collapsible): collapsible component addition * fix(oui-collapsible): example update * fix(oui-collapsible): code clean-up * fix(oui-collapsible): accessibility fixes * fix(oui-collapsible): header replaced with button * fix(oui-collapsible): fix test and update readme * fix(oui-collapsible): fix collapse animation
4 participants
UU-836
Collapsible component added.
Linked to
ovh/ovh-ui-kit#197
ovh-ux/ovh-ui-kit-documentation#94