-
-
Notifications
You must be signed in to change notification settings - Fork 640
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: hover only applies on devices that support it #10698
Conversation
To answer your question, no I don't think those selectors were meant to work like that, and your simpler version is likely sufficient to hit the intention of not inverting icons. |
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.
Awesome! Definitely an upgrade. I love that it helps us standardize on :focus-visible
as well.
Thanks for clarifying! I can push up a patch to update those two usages to use the mixin. |
resolves #9335
Overview
This PR introduces a new
interactive
mixin that makes it easy to apply and share interactive styles on elements while respecting device UX best practices. It currently supports the following features:The original motivation was for hover states, but I added focus/active since there were several selectors that reused the same styles across these states.
Scope
The scope of this work only covers selectors that apply
:hover
. As a result, there are likely selectors that use things like:focus
but not:hover
that have not been modified. I did this to keep the work small and easy to review.Possible Behavior Change
Selectors that were migrated to use the mixin now used
:focus-visible
where they may have used:focus
previously. DIM currently uses a mix of these states. It's possible that this may cause some small visual behavior changes. However, I'd encourage the use offocus-visible
overfocus
anyways mainly because it prevents focus states from appearing stuck on mobile.There are existing usages of
@media (hover: hover)
that I also migrated to use this mixin. As a result, those now use@media (any-hover: hover)
which is a bit more permissive thanhover: hover
(the former matching only the primary input and the latter matching any input). I don't have strong feelings over the use ofhover: hover
vsany-hover: hover
, so if you'd like to usehover: hover
instead I can do that.Outstanding Questions
There are two selectors I intentionally did not migrate. One in dim-button.scss and one in Record.m.scss. I wasn't sure if these selectors are desired as-is.
For example, DIM has this code:
Which compiles out to
I likely don't have all the context here, but it seems like the following would be what you'd want instead:
Are these selectors intended to be this way?