feat(colorwheel): S2 migration by cdransf · Pull Request #3390 · adobe/spectrum-css

cdransf · 2024-11-12T19:43:17Z

Description

CSS-1020

This change migrates the colorwheel component to S2. It adds the --spectrum-colorwheel-border-color-rgb and --spectrum-colorwheel-border-opacity custom properties. It updates --spectrum-colorwheel-border-color to leverage these tokens in an rgba(...) function.

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 steps

Run Storybook locally or open the link for the PR.
Navigate to the colorwheel component.
Verify that no regressions are visible.

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

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

changeset-bot · 2024-11-12T19:43:22Z

🦋 Changeset detected

Latest commit: 0579096

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/colorwheel	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-11-12T19:46:18Z

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

github-actions · 2024-11-12T19:49:09Z

File metrics

Summary

Total size: 1.38 MB^*

Package	Size	Minified	Gzipped
colorwheel	5.55 KB	5.20 KB	1.36 KB

colorwheel

Filename	Head	Minified	Gzipped	Compared to base
index.css	5.55 KB	5.20 KB	1.36 KB	🔴 ⬆ 0.89 KB
metadata.json	2.05 KB	-	-	🔴 ⬆ 0.15 KB

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

marissahuysentruyt

A few questions for you, as usual!

I wonder if we need to refactor the color-wheel-width and color-wheel-minimum-width tokens from our custom tokens/dist. Those values change based on the platform (desktop/mobile), but the designs don't say anything about any size changes between desktop and mobile. In other components, they have noted the size differences. It might be something worth checking with design on. 🤷‍♀️
Do we need to adjust .spectrum-ColorWheel-border at all so that our border is actually transparent, against the color wheel, instead of outside of the color wheel? I'm not sure if the clip path is the right place, but maybe that custom property needs some tweaking? I think S2 would be the place to fix this! (because it's also not like that in main, but looking at S1 designs, it should have been).

Ours (effectively, has a gray border):

In Figma (where the border is "on top" of the color wheel):

Happy to pair on any of my comments if two heads are better than one! 👍

cdransf · 2024-11-14T00:11:58Z

A few questions for you, as usual!

I wonder if we need to refactor the color-wheel-width and color-wheel-minimum-width tokens from our custom tokens/dist. Those values change based on the platform (desktop/mobile), but the designs don't say anything about any size changes between desktop and mobile. In other components, they have noted the size differences. It might be something worth checking with design on. 🤷‍♀️

Do we need to adjust .spectrum-ColorWheel-border at all so that our border is actually transparent, against the color wheel, instead of outside of the color wheel? I'm not sure if the clip path is the right place, but maybe that custom property needs some tweaking? I think S2 would be the place to fix this! (because it's also not like that in main, but looking at S1 designs, it should have been).

Ours (effectively, has a gray border):

In Figma (where the border is "on top" of the color wheel): ...

Happy to pair on any of my comments if two heads are better than one! 👍

I started a thread in the implementations channel about the first question — I'll go ahead and run with design's preference.

Since the border is rendered by the clip path but is associated with the div that wraps the color wheel node, I'm not sure we can fix it by adjusting the clip path (to your point). If we reduce the dimensions of the div or reduce the size of the clip path the "border" would disappear since it would render behind the color wheel. Could we render an inset border and attach it to the color wheel node directly? (Pending design approval and input.) ✨

marissahuysentruyt

Oh! Do we need to revisit the disabled color wheel in high contrast mode? This is what I see on Assistiv Labs. Is this intended? Should there be any sort of background color for the wheel?

cdransf · 2025-01-30T20:28:50Z

Oh! Do we need to revisit the disabled color wheel in high contrast mode? This is what I see on Assistiv Labs. Is this intended? Should there be any sort of background color for the wheel?

Addressed this in c84834: Canvas needed to be CanvasText.

marissahuysentruyt

Nice work! It's so close! I have some smaller suggestions to "leave it better than we found it" for the CSS. I also totally didn't dig into the WHCM stuff initially, and suggested changes that deviated too far from what prod has, so I tried to suggest more accurate changes this time. 🤦‍♀️

In the index.css, could you refactor the min-inline-size so we aren't using the token directly in the style definition? This wasn't something you introduced, but I figured maybe we could fix it up so it matches what our preferences are. Maybe something like
--spectrum-colorwheel-min-inline-size: var(--spectrum-color-wheel-minimum-width)? And then use the min-inline-size custom property in the style definition.

Last but not least, do you think we have any tests to add? I was thinking it wouldn't be a terrible idea to throw a quick isWithColorLoupe: false test into the tests file.

marissahuysentruyt · 2025-02-03T20:12:39Z

-		--highcontrast-colorwheel-border-color-disabled: GrayText;
-		--highcontrast-colorwheel-fill-color-disabled: Canvas;


I totally spoke too soon on the WHCM stuff! I left an updated (more thought-through 😬 ) idea that should replicate what's on prod better now.

Looks like we still need the gradient itself, and then I have a better idea how to replicate this disabled state now:
(this is production when I test in AssistivLabs)

Hmm — took a look at current state in Assistiv and it looks better, but I'm not sure if it's exactly what we want?

Yeah....I think there's a discrepancy between Canvas and CanvasText. I believe swapping this --highcontrast-colorwheel-fill-color-disabled: Canvas; instead of CanvasText should get rid of that white background in the disabled wheel.

jawinn

Nice work adding the custom size version.
A few questions and suggestions here:

jawinn · 2025-02-17T23:48:15Z

+	inset-block: var(--mod-colorwheel-border-width, var(--spectrum-colorwheel-border-width));
+	inset-inline: var(--mod-colorwheel-border-width, var(--spectrum-colorwheel-border-width));


Do you know why this element is inset by the border width originally, and whether we still need this?

I don't know why it was set this way, but we do need it as the conic gradient/wheel content does not display without it.

jawinn

🎉 This should be good to go once things are cleared up in spectrum-two (build issue and all story descriptions not appearing).

marissahuysentruyt

OOOOOOh man, this looks really excellent! Thanks for sticking this one out with all of the high contrast things, and the transparent borders. They all look fabulous!

I left a few non-blocking comments. The one comment I left about the mods listed in the changeset is the only one I'm thinking that maybe we should updated before merging. I'll approve anyways, and leave the rest up to you! Great job on this.

- chore(colorwheel): update property name - chore(colorwheel): wip inset borders - chore(colorwheel): fix lint violations - chore(colorwheel): move mod declarations - chore(colorwheel): drop unused property + update metadata - chore(colorwheel): remove underlying border node + class - chore(colorwheel): remove unused property - fix(colorwheel): add missing mod - fix(colorwheel): comments for updated before spacing/positioning - fix(colorwheel): adopt proper token for border opacity - fix(colorwheel): remove inset border on disabled state - fix(colorwheel): remove global token reference - chore(colorwheel): highlight removed/added mods - chore(colorwheel): update copyright year - chore(colorwheel): add color loupe to default story - fix(colorwheel): correct WHC disabled background color value - chore(colorwheel): update test heading - fix(colorwheel): typos - chore(colorwheel): update token usage - chore(colorwheel): add without loupe test - chore(colorwheel): restore WHC styles - chore(colorwheel): move isWithColorLoupe property - fix(colorwheel): whc disabled background - fix(colorwheel): improve interior border styles - fix(colorwheel): token/classnames in story; remove unnecessary margin - chore(colorwheel): fix inset borders + improve comments - chore(colorwheel): update changeset - chore(colorwheel): add note re --spectrum-color-wheel-color-area-margin token - chore(colorwheel): restore missing mods; naming consistency - chore(colorwheel): add sizing stories - chore(colorwheel): restore comment - chore(colorwheel): inset shorthand - chore(colorwheel): remove unnecessary colorwheel template + story - chore(colorwheel): remove unnecessary custom sizing story from sidebar - chore(colorwheel): add custom sizing test - chore(colorwheel): move comment to docs - fix(colorwheel): after pseudo selector disabled state - fix(colorwheel): remove problematic border width mod - fix(colorwheel): typos + drop sentence

cdransf added size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Nov 12, 2024

cdransf requested review from castastrophe, marissahuysentruyt, pfulton and rise-erpelding November 12, 2024 19:43

cdransf self-assigned this Nov 12, 2024

cdransf changed the title ~~feat(colorwheel): s2 migration~~ feat(colorwheel): S2 migration Nov 12, 2024

marissahuysentruyt reviewed Nov 13, 2024

View reviewed changes

Comment thread components/colorwheel/index.css

Comment thread components/colorwheel/index.css

Comment thread components/colorwheel/index.css Outdated

Comment thread components/colorwheel/index.css

cdransf force-pushed the cdransf/s2-colorwheel-migration branch from eaf2611 to 3a12421 Compare November 14, 2024 00:15

cdransf added the blocked See description and comments for what is blocking this issue label Nov 14, 2024

cdransf force-pushed the cdransf/s2-colorwheel-migration branch 2 times, most recently from 99a6ad6 to f3be898 Compare November 20, 2024 22:30

cdransf requested a review from marissahuysentruyt November 26, 2024 16:41

cdransf removed the blocked See description and comments for what is blocking this issue label Nov 26, 2024

castastrophe force-pushed the spectrum-two branch 3 times, most recently from cdb180d to 27d01df Compare December 4, 2024 14:54

cdransf force-pushed the cdransf/s2-colorwheel-migration branch 2 times, most recently from 7ce5746 to 7af66a2 Compare December 10, 2024 15:18

cdransf force-pushed the cdransf/s2-colorwheel-migration branch 3 times, most recently from 9cb357e to 70603e0 Compare December 19, 2024 22:38

castastrophe force-pushed the spectrum-two branch 2 times, most recently from c93daf7 to e393c32 Compare December 27, 2024 18:11

marissahuysentruyt reviewed Jan 29, 2025

View reviewed changes

cdransf force-pushed the cdransf/s2-colorwheel-migration branch 3 times, most recently from 378549e to f2aafaa Compare January 30, 2025 15:48

cdransf force-pushed the cdransf/s2-colorwheel-migration branch 3 times, most recently from c9d910f to 086faac Compare February 3, 2025 16:30

cdransf requested a review from marissahuysentruyt February 3, 2025 16:31

marissahuysentruyt requested changes Feb 4, 2025

View reviewed changes

cdransf force-pushed the cdransf/s2-colorwheel-migration branch 2 times, most recently from a984eb6 to 61d7526 Compare February 4, 2025 22:12

castastrophe force-pushed the spectrum-two branch 2 times, most recently from 56c6806 to 793571c Compare February 5, 2025 17:33

cdransf force-pushed the cdransf/s2-colorwheel-migration branch from 1f3d572 to eb8c57f Compare February 5, 2025 19:20

jawinn suggested changes Feb 10, 2025

View reviewed changes

Comment thread components/colorwheel/index.css Outdated

Comment thread components/colorwheel/index.css

Comment thread components/colorwheel/dist/metadata.json

Comment thread components/colorwheel/stories/colorwheel.stories.js

jawinn suggested changes Feb 17, 2025

View reviewed changes

jawinn approved these changes Feb 25, 2025

View reviewed changes

marissahuysentruyt approved these changes Feb 27, 2025

View reviewed changes

Comment thread .changeset/giant-windows-smoke.md Outdated

Comment thread .changeset/giant-windows-smoke.md Outdated

Comment thread components/colorwheel/stories/colorwheel.stories.js Outdated

Comment thread components/colorwheel/stories/colorwheel.stories.js Outdated

github-actions Bot mentioned this pull request Feb 27, 2025

chore: release (next) #3583

Merged

castastrophe mentioned this pull request Apr 15, 2025

feat(tokens,card): update to @adobe/spectrum-tokens@13.3.0 #3669

Merged

12 tasks

This was referenced May 16, 2025

chore(swatch,menu,swatchgroup): fix reverted metadata assets #3752

Merged

chore: merge latest updates from main #3777

Merged

castastrophe mentioned this pull request Jul 25, 2025

ci: chromatic runs for S2 [CSS-1233] #4070

Merged

3 tasks

castastrophe mentioned this pull request Aug 6, 2025

fix: minor linting updates #4113

Merged

11 tasks

This was referenced Aug 27, 2025

chore(popover): wrapper placement in story #4173

Closed

feat(storybook): chromatic a11y testing #4180

Closed

chore: updating lint-staged file inheritance order #4193

Closed

castastrophe mentioned this pull request Oct 22, 2025

feat(badge): remove modifiers from the API [SWC-1264] #4283

Merged

14 tasks

		--highcontrast-colorwheel-border-color-disabled: GrayText;
		--highcontrast-colorwheel-fill-color-disabled: Canvas;

		inset-block: var(--mod-colorwheel-border-width, var(--spectrum-colorwheel-border-width));
		inset-inline: var(--mod-colorwheel-border-width, var(--spectrum-colorwheel-border-width));

Conversation

cdransf commented Nov 12, 2024

Description

How and where has this been tested?

Validation steps

Regression testing

To-do list

Uh oh!

changeset-bot Bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

colorwheel

Uh oh!

marissahuysentruyt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdransf commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marissahuysentruyt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdransf commented Jan 30, 2025

Uh oh!

marissahuysentruyt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marissahuysentruyt Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

cdransf Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdransf Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jawinn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jawinn Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdransf Feb 19, 2025

changeset-bot Bot commented Nov 12, 2024 •

edited

Loading

github-actions Bot commented Nov 12, 2024 •

edited

Loading

github-actions Bot commented Nov 12, 2024 •

edited

Loading

cdransf commented Nov 14, 2024 •

edited

Loading

marissahuysentruyt left a comment •

edited

Loading

marissahuysentruyt left a comment •

edited

Loading

marissahuysentruyt Feb 5, 2025 •

edited

Loading

jawinn Feb 17, 2025 •

edited

Loading

marissahuysentruyt left a comment •

edited

Loading