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

refactor: better focus-visible a11y #1017

Open
wants to merge 7 commits into
base: canary
Choose a base branch
from

Conversation

Barsnes
Copy link
Contributor

@Barsnes Barsnes commented Dec 29, 2024

This PR aims to tackle a11y focus visibility.
Currently some items does not show visible focus, and some items uses the wrong semantic element.

This does not enhance anything else related to a11y, except a few instances where doing so improves focusing elements.

@Barsnes Barsnes marked this pull request as ready for review December 29, 2024 05:43
@Barsnes Barsnes requested a review from Siumauricio as a code owner December 29, 2024 05:43
@190km
Copy link
Contributor

190km commented Dec 29, 2024

all the changes look good, just for the input I imagine it was removed originally to avoid destroying the UX, because the focus ring around the input doesn't look very good.

@Barsnes
Copy link
Contributor Author

Barsnes commented Dec 29, 2024

all the changes look good, just for the input I imagine it was removed originally to avoid destroying the UX, because the focus ring around the input doesn't look very good.

Thanks for the feedback. I'd argue the input should have some focus indication, but I'll gladly make changes to the focus ring on the input, perhaps make it smaller?

@190km
Copy link
Contributor

190km commented Dec 29, 2024

all the changes look good, just for the input I imagine it was removed originally to avoid destroying the UX, because the focus ring around the input doesn't look very good.

Thanks for the feedback. I'd argue the input should have some focus indication, but I'll gladly make changes to the focus ring on the input, perhaps make it smaller?

yes, I think it would look better if it was smaller and darker

@Barsnes
Copy link
Contributor Author

Barsnes commented Dec 29, 2024

all the changes look good, just for the input I imagine it was removed originally to avoid destroying the UX, because the focus ring around the input doesn't look very good.

Thanks for the feedback. I'd argue the input should have some focus indication, but I'll gladly make changes to the focus ring on the input, perhaps make it smaller?

yes, I think it would look better if it was smaller and darker

Here are some suggestions:
1: focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-1
image

2: focus-visible:ring-1 focus-visible:ring-ring
image

3: focus-visible:ring-2 focus-visible:ring-ring
image

4: focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-gray-400
image

I have committed option 3 to the branch

@Barsnes
Copy link
Contributor Author

Barsnes commented Dec 30, 2024

I have noticed a few spots I missed, so will set this back to draft for now.

@Barsnes Barsnes marked this pull request as draft December 30, 2024 22:11
@Barsnes Barsnes marked this pull request as ready for review December 30, 2024 22:56
@Siumauricio
Copy link
Contributor

I don't like to much the colors, using ring-border feels more right according to the colors instead of white color
Screenshot 2025-01-01 at 7 18 19 PM

@Barsnes
Copy link
Contributor Author

Barsnes commented Jan 2, 2025

I don't like to much the colors, using ring-border feels more right according to the colors instead of white color Screenshot 2025-01-01 at 7 18 19 PM

This is up to you in the end, so I have gone ahead and committed ring-border for focus-visible on the input.
Let me know if there are any other changes you'd like

@Siumauricio
Copy link
Contributor

@Barsnes Go ahead with the changes

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.

3 participants