-
Notifications
You must be signed in to change notification settings - Fork 198
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
Lunarfusion/css 117 progress circle core tokens #1488
Lunarfusion/css 117 progress circle core tokens #1488
Conversation
🚀 Deployed on https://pr-1488--spectrum-css.netlify.app |
eec318e
to
ba2a5ec
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.
other than the question about the latest tokens "@spectrum-css/tokens": "^1.0.1"
(do yarn install
please to test it out) this looks really good! Great work.
I think the VRT will fail as there are a couple of class-names renamed. We need to expect that in the test.
54c65d6
to
a1dee23
Compare
@lunarfusion @bernhard-adobe - we actually have Progress Circle ignored in the VRT scenarios due to BackstopJS not being able to handle animations, so this one won't even run. |
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.
@lunarfusion nice work! Let's remove the changes to the .gitignore
and I think we're good to go on this one.
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.
Great work, approved
d8fa0d0
to
a2545a0
Compare
Released: |
@jnurthen - would you mind taking a look at the changes to the Progress Circle here to let @lunarfusion know if the Windows High Contrast Mode styles are acceptable? Thank you! |
@pfulton - I think I made a mistake when I fixed this one originally. The Highlight colour is not guaranteed to be a visible colour combination with the track (which I think gets ButtonBorder/ButtonText by default). I think a border between the 2 could resolve this - or maybe using HighlightText as one of the colours or another way? However, I wouldn't want to block this PR on that as it is not a new error. IMO This can be tracked in a separate (lower priority) Issue as the visibiity of the progress on a progresscircle is likely low priority. |
@jnurthen The change that I initially made to WHCM was to ensure the track and the fill have contrast. I did not override the track color for WHCM, but I did override the fill with keyword "Highlight". Here is the VRT result. Note that the VRTs appear to be using Mac emulation of WHCM. Our assessment of the difference between Mac emulation and actual default WHCM colors on a windows machine is:
Edit: The emulation uses dark mode, but in light mode I can see what you pointed out in your comment. The track looks black and the fill looks dark blue. One helpful change is to add "forced-color-adjust: none;" to the WHCM media query, which gives us this: Would you like any changes to track or fill color keywords or to the delineation of track and fill based on this info? I can provide a comprehensive visual of our Mac emulation vs PC results if that might help. |
These are the 4 high contrast themes that windows (10) provides. HC #1### HC #2 ### HC Dark ### HC LightThese were tested using Assistiv Labs which is running windows. I don't like using forced-color-adjust: none here as that could lead to the track completely disappearing which I don't think is any better than the current situation. I was playing around with it and changing the border-style on the track to double might do the trick! |
a2545a0
to
470eb10
Compare
Released: |
BREAKING CHANGE: migrates progress circle to core tokens
8719f01
to
d958cbd
Compare
Description
BREAKING CHANGE: This migrates the Progress Circle component to core tokens.
How and where has this been tested?
Screenshots
To-do list