feat(colorwheel): S2 migration #3390
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@spectrum-css/colorwheel": major | ||
| --- | ||
|
|
||
| # colorwheel S2 migration | ||
|
|
||
| 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. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,33 +11,33 @@ | |||||
| * governing permissions and limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| .spectrum-ColorWheel { | ||||||
| --spectrum-colorwheel-width: var(--spectrum-color-wheel-width); | ||||||
| --spectrum-colorwheel-min-width: var(--spectrum-color-wheel-minimum-width); | ||||||
| --spectrum-colorwheel-height: var(--spectrum-color-wheel-width); | ||||||
|
|
||||||
| /* @TODO: leveraging the rgb token in this case to achieve the desired border color implementation as rgb + opacity are required by the `rgba` function. */ | ||||||
| --spectrum-colorwheel-border-color-rgb: var(--spectrum-gray-1000-rgb); | ||||||
cdransf marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| /* @TODO: using explicit value as `color-wheel-border-opacity` is undefined. */ | ||||||
| --spectrum-colorwheel-border-opacity: 0.1; | ||||||
cdransf marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Was this defined in a recent tokens update? https://github.com/adobe/spectrum-tokens/blob/5f67e9c2d0fe52d3f488cead97f41b427f327215/packages/tokens/src/color-component.json#L481 beta.55 maybe?
Suggested change
|
||||||
| --spectrum-colorwheel-border-color: rgba(var(--spectrum-colorwheel-border-color-rgb), var(--spectrum-colorwheel-border-opacity)); | ||||||
|
|
||||||
| --spectrum-colorwheel-fill-color-disabled: var(--mod-colorwheel-fill-color-disabled, var(--spectrum-disabled-background-color)); | ||||||
|
|
||||||
| --spectrum-colorwheel-border-width: var(--spectrum-border-width-100); | ||||||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. ✨ |
||||||
| --spectrum-colorwheel-track-width: var(--mod-colorwheel-track-width, var(--spectrum-color-control-track-width)); | ||||||
|
There was a problem hiding this comment. I just noticed that Is that something we should change or fix? It might be intentional with these two, but I wanted to bring it up to double check! |
||||||
|
|
||||||
| --spectrum-colorwheel-colorarea-margin: var(--spectrum-color-wheel-color-area-margin); | ||||||
|
|
||||||
| /* stylelint-disable-next-line spectrum-tools/no-unused-custom-properties -- used with JS in calculating the clip-path paths and colorarea-container-size */ | ||||||
| --_track-width: var(--spectrum-colorwheel-track-width); | ||||||
| /* stylelint-disable-next-line spectrum-tools/no-unused-custom-properties -- used with JS in calculating the clip-path paths and colorarea-container-size */ | ||||||
| --_border-width: var(--spectrum-colorwheel-border-width); | ||||||
|
|
||||||
| position: relative; | ||||||
| display: block; | ||||||
| min-inline-size: var(--mod-colorwheel-min-width, var(--spectrum-colorwheel-min-width)); | ||||||
| inline-size: var(--spectrum-colorwheel-width); | ||||||
| block-size: var(--spectrum-colorwheel-height); | ||||||
| user-select: none; | ||||||
|
|
@@ -75,7 +75,7 @@ | |||||
| display: flex; | ||||||
| align-items: center; | ||||||
| justify-content: center; | ||||||
| margin: var(--mod-colorwheel-colorarea-margin, var(--spectrum-colorwheel-colorarea-margin)); | ||||||
| } | ||||||
|
|
||||||
| .spectrum-ColorWheel-slider { | ||||||
|
|
@@ -130,3 +130,13 @@ | |||||
| background: var(--highcontrast-colorwheel-fill-color-disabled, var(--spectrum-colorwheel-fill-color-disabled)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /* Windows High Contrast Mode */ | ||||||
| @media (forced-colors: active) { | ||||||
| .spectrum-ColorWheel { | ||||||
| --highcontrast-colorwheel-border-color-disabled: GrayText; | ||||||
| --highcontrast-colorwheel-fill-color-disabled: Canvas; | ||||||
|
|
||||||
| forced-color-adjust: none; | ||||||
| } | ||||||
| } | ||||||
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.
Totally non-blocking, but what do you think about renaming this from
--spectrum-colorwheel-min-widthto--spectrum-colorwheel-min-inline-size? I've seen that in a few other places, but I'm not sure if the more "logical" naming is a team preference. Picker has some variables named with the logical naming. 🤷♀️ Just a thought. I guess if we rename this one variable, maybe it makes sense to rename the others, too.