-
Notifications
You must be signed in to change notification settings - Fork 194
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(slider): focus state matches spec, supports forced-colors #2217
Conversation
🚀 Deployed on https://pr-2217--spectrum-css.netlify.app |
fb30aba
to
ce7ed65
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.
It's looking much closer to the intended spec now. A few questions and suggestions below.
There doesn't seem to be any indication in the WHCM design spec about how to handle the "Filled", , "Filled-Offset", and "Range". I think perhaps that at least for focus and hover, the darker portion should use the Highlight color (in the PR you can see the reverse of this on "Filled-Offset").
Ideally there'd be a way to differentiate the darkened part of the range by default in WHCM as well, but I'm not sure about the best way; possibly a thicker track since there isn't another appropriate system color to use. But that would need some followup design/a11y involvement/approval.
I also noticed that there are some existing high contrast styles for the ramp that we may want to move next to the rest of the high contrast styles.
.spectrum-Slider { | ||
&.is-disabled { | ||
cursor: default; | ||
|
||
.spectrum-Slider-handle { | ||
cursor: default; | ||
pointer-events: none; | ||
} | ||
|
||
.spectrum-Slider-tickLabel { | ||
color: var(--highcontrast-slider-label-text-color-disabled, var(--mod-slider-label-text-color-disabled, var(--spectrum-slider-label-text-color-disabled))); | ||
} | ||
} | ||
} |
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.
Relocated this to an existing spectrum-Slider.is-disabled
selector on new line 628
@media (forced-colors: active) { | ||
/* forced-color-adjust required to ensure the "circle" around the handle is transparent */ | ||
forced-color-adjust: none; | ||
box-shadow: 0 0 0 var(--spectrum-slider-handle-gap) ButtonFace; | ||
border-color: ButtonText; | ||
background-color: ButtonFace; | ||
} |
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.
We can rely on the --highcontrast
custom properties to do this instead.
03f063d
to
cb33dfd
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.
Looks good to me! Thanks for those updates.
I only have one small recommendation to move a few lines of CSS.
50e7fe7
to
c111fdd
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.
This looks great. Thanks for taking care of this.
@najikahalsema @majornista This work should address #2200. Would you mind taking a look to be sure that we've fixed everything? PR deploy is here: https://pr-2217--spectrum-css.netlify.app/ |
Looks good to me! Thanks for doing this work. |
c111fdd
to
f070c71
Compare
f070c71
to
a8cca63
Compare
@castastrophe Noting that this one will have some Docs changes! |
Description
This PR addresses issue #2200.
.spectrum-Slider
and.spectrum-Slider-handle
) but needed to be explicitly set on.spectrum-Slider-controls
to take effect since this class has a specifiedcursor: pointer
when the slider isn't disabled.How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation
Highlight
colorHighlight
to match the specRegression testing
Validate:
To-do list
I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨