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: Improve accessibility of the theme switcher #160

Merged
merged 7 commits into from
May 18, 2020
Merged

Conversation

palant
Copy link
Collaborator

@palant palant commented May 18, 2020

The keyboard accessibility of the theme switcher isn't great right now. When you press Tab to go through menu entries, the focus gets to the theme switcher but no focus ring appears - you don't see where the focus is. Also, pressing Enter (which is what you would do with the other menu items) doesn't do anything, you have to press Space because it is a checkbox which isn't exactly obvious.

So I made the theme switcher a link like the other entries, this makes its behavior consistent. If we are worried about screen readers, we might want to add some hidden text to it as well. Given that this is the switch for the visual theme however, adding aria-hidden="true" to the entire element might make more sense.

@palant palant requested a review from reuixiy May 18, 2020 08:57
@palant palant mentioned this pull request May 18, 2020
@reuixiy
Copy link
Owner

reuixiy commented May 18, 2020

I have to admit that this is a better approach.

Side-notes: We should keep the element and its data-hide attribute, so we can remove extra margin by appending display: none to its parent node <li>.

2020-05-18-19-46-30

@reuixiy
Copy link
Owner

reuixiy commented May 18, 2020

Side-notes: This is a requirement from https://github.com/gohugoio/hugoThemes#resources

@palant
Copy link
Collaborator Author

palant commented May 18, 2020

I think that a CSS-only solution with :blank will work as well, let me try...

@palant
Copy link
Collaborator Author

palant commented May 18, 2020

Done. The downside here is: whitespace isn't allowed either, or it won't be hidden. The other solution would be not emitting the list item in the first place, something like:

{{ $Content := partial "components/dark-mode.html" $ctx }}
{{ with $Content }}
    <li class="menu-item">{{ $Content }}</li>
{{ end }}

@palant palant requested a review from reuixiy May 18, 2020 12:26
@reuixiy
Copy link
Owner

reuixiy commented May 18, 2020

The other solution would be not emitting the list item in the first place

This! 🤯

@palant
Copy link
Collaborator Author

palant commented May 18, 2020

This! 🤯

Ok, I take it that this is your preferred solution. Done.

@palant palant merged commit e5e6230 into master May 18, 2020
@palant palant deleted the theme-switcher-a11y branch May 18, 2020 12:54
palant added a commit that referenced this pull request May 18, 2020
ulmefors pushed a commit to ulmefors/hugo-theme-meme that referenced this pull request Nov 22, 2020
* fix: Improve accessibility of the theme switcher

* Added some styles back, these aren't redundant in "center" header mode

* Restore data-hide attribute

* chore: update resources folder with example config.toml

* Hide theme switcher's parent via CSS when theme switcher isn't visible

* chore: Fixed whitespace

* Don't emit the theme switcher's parent at all if theme switcher is hidden

Co-authored-by: reuixiy <reuixiy@gmail.com>
ulmefors pushed a commit to ulmefors/hugo-theme-meme that referenced this pull request Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants