Skip to content
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

feat(colorwheel): S2 migration #3390

Open
wants to merge 2 commits into
base: spectrum-two
Choose a base branch
from

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Nov 12, 2024

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

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

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. 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. ✨

@cdransf 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 cdransf self-assigned this Nov 12, 2024
Copy link

changeset-bot bot commented Nov 12, 2024

🦋 Changeset detected

Latest commit: f3be898

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

Copy link
Contributor

github-actions bot commented Nov 12, 2024

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

Copy link
Contributor

github-actions bot commented Nov 12, 2024

File metrics

Summary

Total size: 4.29 MB*
Total change (Δ): ⬆ 0.60 KB (0.01%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
colorwheel 4.95 KB ⬆ 0.17 KB

Details

colorwheel

File Head Base Δ
index-base.css 4.95 KB 4.79 KB ⬆ 0.17 KB (3.53%)
index-vars.css 4.95 KB 4.79 KB ⬆ 0.17 KB (3.53%)
index.css 4.95 KB 4.79 KB ⬆ 0.17 KB (3.53%)
metadata.json 2.05 KB 1.97 KB ⬆ 0.08 KB (4.06%)
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf changed the title feat(colorwheel): s2 migration feat(colorwheel): S2 migration Nov 12, 2024
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions for you, as usual!

  1. 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. 🤷‍♀️
  2. 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):
Screenshot 2024-11-13 at 4 17 58 PM

In Figma (where the border is "on top" of the color wheel):
Screenshot 2024-11-13 at 4 18 19 PM

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

components/colorwheel/index.css Show resolved Hide resolved
components/colorwheel/index.css Show resolved Hide resolved
components/colorwheel/index.css Show resolved Hide resolved
--spectrum-colorwheel-border-color-rgb: var(--spectrum-gray-1000-rgb);
--spectrum-colorwheel-border-opacity: 0.1;
--spectrum-colorwheel-border-color: rgba(var(--spectrum-colorwheel-border-color-rgb), var(--spectrum-colorwheel-border-opacity));
--spectrum-colorwheel-border-width: var(--spectrum-border-width-100);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm...so everything checks out right now. The border width is 1px, and if I inspect the Figma component, it also has a 1px border.

Screenshot 2024-11-13 at 4 16 07 PM

However, I read the blurb in the desktop file, and it says that the color wheel is supposed to have a 2px border? You may have already done this, but I was going to suggest we double check this with design. This change isn't listed in the changelog in Figma, so I think maybe this is outdated information. Maybe design would want to correct that 2px thing? Or update border-width-100?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added a comment about this to the thread I dropped into the implementations channel. I'll get both updated pending some clarification. ✨

@cdransf
Copy link
Member Author

cdransf commented Nov 14, 2024

A few questions for you, as usual!

  1. 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. 🤷‍♀️
  2. 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.) ✨

@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from eaf2611 to 3a12421 Compare November 14, 2024 00:15
@cdransf cdransf added the blocked See description and comments for what is blocking this issue label Nov 14, 2024
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from 3a12421 to 99a6ad6 Compare November 18, 2024 23:51
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from 99a6ad6 to f3be898 Compare November 20, 2024 22:30
@cdransf cdransf removed the blocked See description and comments for what is blocking this issue label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants