-
Notifications
You must be signed in to change notification settings - Fork 196
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
base: spectrum-two
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: f3be898 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
🚀 Deployed on https://pr-3390--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.29 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailscolorwheel
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
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.
A few questions for you, as usual!
- I wonder if we need to refactor the
color-wheel-width
andcolor-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 inmain
, 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! 👍
--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); |
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.
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.
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
?
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.
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. ✨
I started a thread in the implementations channel about the first question — I'll go ahead and run with design's preference.
|
eaf2611
to
3a12421
Compare
3a12421
to
99a6ad6
Compare
99a6ad6
to
f3be898
Compare
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 anrgba(...)
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
colorwheel
component.Regression testing
Validate:
To-do list