-
Notifications
You must be signed in to change notification settings - Fork 201
feat(dial)!: migrate to use @adobe/spectrum-tokens
#1971
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
Conversation
🚀 Deployed on https://pr-1971--spectrum-css.netlify.app |
450c803
to
823cf90
Compare
823cf90
to
4882705
Compare
4882705
to
aa00c52
Compare
aa00c52
to
beae7cc
Compare
beae7cc
to
9c13b6c
Compare
9c13b6c
to
9b3e833
Compare
9b3e833
to
e981892
Compare
85d9b3f
to
0cc89ee
Compare
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.
Couple of small questions and one issue with the custom token not being applied.
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.
This is looking pretty good.
My only questions are around focus and keyboard focus, which also apply to what is in prod. Right now there is no visible focus when tabbing to the dial. On the docs page, is-keyboardFocused
is applied by the JS there, which does not trigger the .is-focused
styles (the dark grey visible after click and drag). Rather than using .is-focused
, should this be using :focus
or :focus-visible
? The docs currently are allowing focus on the handle with tabindex='0'
, which allows those pseudo classes to be activated.
I'm also wondering about disabled showing a hover background color. It does it that way in prod, but it seems odd because it makes it look interactive. And it doesn't seem to match how other components' disabled states behave.
The updates for disabled and focus look great! One thing I am noticing is that tab focus disappears for one stop as it appears to go from the handle to the range input itself. I'm not sure if we should put a |
BREAKING CHANGE: migrates the Dial component to `@adobe/spectrum-tokens`
Uses correct colors & removes duplicate properties
41996d3
to
942be55
Compare
Description
Migrates the Dial component to use
@adobe/spectrum-tokens
instead of the legacy "vars" packages.This component does not have an updated spec file from design, nor does it have new tokens defined for it. This is a "DIY migration".
How and where has this been tested?
Screenshots
To-do list