spectrum-css icon indicating copy to clipboard operation
spectrum-css copied to clipboard

Lunarfusion/css 117 progress circle core tokens

Open lunarfusion opened this issue 3 years ago • 4 comments

Description

BREAKING CHANGE: This migrates the Progress Circle component to core tokens.

  • Jira issue CSS-117 https://jira.corp.adobe.com/browse/CSS-117

How and where has this been tested?

  • Chrome 103 for macOS
  • Firefox 102 for macOS
  • Safari 15.5 for macOS
  • How this was tested: comparing local build of http://localhost:3000/docs/progresscircle.html with XD redline components batch and with https://spectrum.adobe.com/page/progress-circle/

Screenshots

Screen Shot 2022-08-01 at 11 08 10 AM Screen Shot 2022-08-01 at 11 08 18 AM Screen Shot 2022-08-01 at 2 33 04 PM

To-do list

  • [ ] If my change impacts other components, I have tested to make sure they don't break.
  • [x] If my change impacts documentation, I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have tested these changes in Windows High Contrast mode.
  • [ ] This pull request is ready to merge.

lunarfusion avatar Aug 01 '22 15:08 lunarfusion

🚀 Deployed on https://pr-1488--spectrum-css.netlify.app

github-actions[bot] avatar Aug 01 '22 15:08 github-actions[bot]

I think the VRT will fail as there are a couple of class-names renamed. We need to expect that in the test.

@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.

pfulton avatar Aug 05 '22 19:08 pfulton

VRT test: https://spectrum-visual-regression.ci.corp.adobe.com/view/Spectrum%20CSS/job/css-vrt-test/35/artifact/backstop_data/html_report/index.html 2022-08-10 -- 1488 - progress-circle.zip

bernhard-adobe avatar Aug 11 '22 05:08 bernhard-adobe

Released: 2.0.0-beta.0

pfulton avatar Aug 24 '22 14:08 pfulton

@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 avatar Sep 29 '22 20:09 pfulton

@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). This is particularly apparent when viewing the High Contrast (light) theme. Screen Shot 2022-09-29 at 2 17 13 PM

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 avatar Sep 29 '22 21:09 jnurthen

@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.

Before: 188222934-87765339-1058-4c1f-b4b9-29d03f645424

After: 188222970-d73f40f3-b829-4858-8576-4d552dfb3503

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:

  • Highlight color that looks bright blue (to me) on Mac emulation (left in the visual below)
  • Highlight color that looks pale purple (to me) on a PC that a colleague tested for us (right in the visual below) Screen Shot 2022-09-30 at 2 45 01 PM

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. Screen Shot 2022-09-30 at 3 01 05 PM Here is a visual of Mac emulation of Highlight in light mode (left) and PC test result (right): Screen Shot 2022-09-30 at 3 02 54 PM

One helpful change is to add "forced-color-adjust: none;" to the WHCM media query, which gives us this: Screen Shot 2022-09-30 at 3 05 32 PM Screen Shot 2022-09-30 at 3 05 39 PM

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.

lunarfusion avatar Sep 30 '22 18:09 lunarfusion

These are the 4 high contrast themes that windows (10) provides.

HC #1

Screen Shot 2022-09-30 at 12 23 59 PM ### HC #2 Screen Shot 2022-09-30 at 12 24 14 PM ### HC Dark Screen Shot 2022-09-30 at 12 23 12 PM ### HC Light Screen Shot 2022-09-30 at 12 23 38 PM

These 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!

Screen Shot 2022-09-30 at 12 38 05 PM Screen Shot 2022-09-30 at 12 38 17 PM

jnurthen avatar Sep 30 '22 19:09 jnurthen

Released: @spectrum-css/[email protected]

pfulton avatar Oct 03 '22 15:10 pfulton