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

Use focus outlines instead of rings and set a default #384

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

robdekort
Copy link
Contributor

Changes proposed in this pull request:

  • Move from focus rings to outlines
  • Set a default and remove all current utilities

@klickreflex
Copy link
Contributor

klickreflex commented Mar 1, 2024

I welcome getting rid of those inaccessible and hard to change ring utilities 👍
Been removing those for a while and replaced them with a variation of my focus and form base stylesheet:

https://gist.github.com/klickreflex/e2d444c15553b2f00545f9dd902c0da7

I've found that using currentColor as the default outline color is a nice way to have it behave as it should most of the time, and for different needs I usually use helper classes such as .focus-black, e.g. for white text on dark background buttons. Using CSS variables also makes it easy to override settings on a component (or media query) level.

To adapt the same defaults to form fields, I needed to override some of TW's base styling. This way it works way better for me.

@robdekort
Copy link
Contributor Author

Brilliant. Exactly the feedback I was looking for. Will merge this in.

@sjclark
Copy link
Contributor

sjclark commented Mar 1, 2024

Huh weird, I honestly had no idea that ring wasn't considered particularly accessible. Just had a bit of a look into it. Any particular reason why all the default Tailwind Forms examples use ring instead of outline? Is it just a legacy thing? 🤷

@robdekort
Copy link
Contributor Author

I think so, yeah.

@klickreflex
Copy link
Contributor

Probably a combination of not having been able to style outlines properly (which luckily has changed) and, to give them the benefit of the doubt, unawareness it's an issue (many otherwise a11y aware devs don't know about it in my experience)

@robdekort
Copy link
Contributor Author

Ok, updated the PR. Please check it out.

@robdekort robdekort merged commit 180645a into main Mar 1, 2024
1 check passed
@robdekort robdekort deleted the feature/focus-outlines branch March 1, 2024 13:28
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