Skip to content
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

Fix accessibility of app navigation toggle #1843

Closed
wants to merge 3 commits into from

Conversation

jancborchardt
Copy link
Contributor

hover/focus feedback before:
menu button focus feedback before

hover/focus feedback after:
menu button after

@jancborchardt jancborchardt added bug Something isn't working 3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component design Design, UX, interface and interaction design accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Apr 13, 2021
@jancborchardt jancborchardt changed the title Increase visibility of app navigation toggle hover/focus state Fix accessibility of app navigation toggle Apr 13, 2021
@jancborchardt
Copy link
Contributor Author

Also added the missing aria-label to the AppNavigationToggle, but couldn’t figure out how to make it translatable. (Tried via props and also via :aria-label="open ? t('Close navigation') : t('Open navigation')" but both just resulted in empty aria-label.)

@raimund-schluessler
Copy link
Contributor

Also added the missing aria-label to the AppNavigationToggle, but couldn’t figure out how to make it translatable. (Tried via props and also via :aria-label="open ? t('Close navigation') : t('Open navigation')" but both just resulted in empty aria-label.)

@jancborchardt You need to import the l10n mixin like so (maybe adjust the path):
https://github.com/nextcloud/nextcloud-vue/blob/b1d53388b6aaae47a5609f2dc5fe476aca821c1f/src/components/SettingsSelectGroup/SettingsSelectGroup.vue#L48

and use it in the component like so:
https://github.com/nextcloud/nextcloud-vue/blob/b1d53388b6aaae47a5609f2dc5fe476aca821c1f/src/components/SettingsSelectGroup/SettingsSelectGroup.vue#L54

Then you can use t('string') in the template:
https://github.com/nextcloud/nextcloud-vue/blob/b1d53388b6aaae47a5609f2dc5fe476aca821c1f/src/components/SettingsSelectGroup/SettingsSelectGroup.vue#L38

@jancborchardt jancborchardt force-pushed the design/navigation-toggle-focus branch from aa2f686 to 89daf40 Compare April 13, 2021 15:11
@jancborchardt
Copy link
Contributor Author

Thanks a lot @raimund-schluessler (and @marcelklehr via chat too), works now and ready to review! :)

@jancborchardt jancborchardt force-pushed the design/navigation-toggle-focus branch from 89daf40 to 6ef930b Compare April 13, 2021 16:48
Signed-off-by: Raimund Schlüßler <[email protected]>
@raimund-schluessler
Copy link
Contributor

@jancborchardt I took the liberty to fix the warning and import error on this branch. It works as it should now.

I am just not so sure about the background-color, as it differs from what we use on other Actions. And thinking about that, I figured that it would actually make sense to simply use an ActionButton for the AppNavigationToggle. So I created another PR which solves the same issue by using an ActionButton, see #1844.

@raimund-schluessler
Copy link
Contributor

Closing in favor of #1844.

@jancborchardt jancborchardt deleted the design/navigation-toggle-focus branch April 15, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working design Design, UX, interface and interaction design feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants