-
Notifications
You must be signed in to change notification settings - Fork 46
Enable keyboard interaction for nav toggles #106
Conversation
assets/js/components/tree.js
Outdated
@@ -63,6 +63,7 @@ class TreeCollection { | |||
this._tree = tree; | |||
this._el = $(el); | |||
this._toggle = this._el.find('> [data-role="toggle"]'); | |||
this._toggleBtn = this._toggle.find('button'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's weird that the actual clickable element is the h4, not the button. it would make more sense to move the data-role
attribute to the button and use the button's focus etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense - ended up removing the h4 entirely to simplify styling
assets/scss/components/_tree.scss
Outdated
@@ -43,14 +43,25 @@ | |||
background-color: $color-ui-hover; | |||
} | |||
|
|||
span { | |||
&:focus-within { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not supported in some older browsers that we should support with our browserslist config (which we don't actually have, since we use the default config)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, no longer relying on this
views/macros/navigation.nunj
Outdated
@@ -14,9 +14,9 @@ | |||
{% if item.isCollection or (item.isComponent and not item.isCollated and item.variants().filter('isHidden', false).size > 1) %} | |||
<li class="Tree-item Tree-collection Tree-depth-{{ depth }}" data-behaviour="collection" id="tree-{{ root.name }}-collection-{{ item.handle }}"> | |||
<h4 class="Tree-collectionLabel" data-role="toggle"> | |||
<span>{{ item.label }}</span> | |||
<button aria-expanded="false" aria-controls="tree-{{ root.name }}-collection-{{ item.handle }}-items">{{ item.label }}</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the aria-expanded="false"
attribute is wrong i think since the collections are closed with js on page load, so this should default to true
here and be switched when the collection is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
Thanks for the feedback @Risker! |
Thank you! |
Should fix #53
<h4>
to<button>
in navigation togglesaria-expanded
andaria-controls
attributes to navigation togglesid
to collection item containers to link toaria-controls