-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use ActionButton for AppNavigationToggle #1844
Conversation
b82043c
to
b08b445
Compare
b08b445
to
9abd4a6
Compare
9abd4a6
to
dec731c
Compare
dec731c
to
5f5290f
Compare
Signed-off-by: Jan C. Borchardt <[email protected]> Signed-off-by: Raimund Schlüßler <[email protected]>
5f5290f
to
e42443d
Compare
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.
When testing with a screenreader, the aria label seems to be duplicated, it reads:
navigation, open navigation collapsed pushbutton, open navigation
And when the navigation is open:
navigation, close navigation expanded pushbutton, close navigation
So it seems we should actually not have the aria-label because the "navigation" already comes from the role="navigation"
on the parent. Then it would just be "navigation, expanded pushbutton".
@jancborchardt Would it be possible that you adjust that? I don't have a screenreader at hand. |
I could try – but which system are you on? If you use GNOME, you can simply go to "Universal Access" and activate the screenreader from there. :) |
…f the ActionButton Signed-off-by: Jan C. Borchardt <[email protected]>
Fixed it :) @raimund-schluessler the issue was simply that the |
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.
Works great now! :)
@skjnldsv could you check and approve if your change request is addressed? :) |
How about a backport here? 😉 |
This is another approach to #1843. We simply use
ActionButton
for theAppNavigationToggle
now and let it do the heavy lifting.hover/focus feedback after: