-
Notifications
You must be signed in to change notification settings - Fork 201
feat(swatch+swatchgroup): S2 migration #3677
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
base: spectrum-two
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 343a87b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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-3677--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.38 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsswatch
swatchgroup
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
ffa2cad
to
37bccbb
Compare
91c31d2
to
d0944eb
Compare
9ca18cb
to
f3f8289
Compare
02c2cca
to
044c357
Compare
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.
Hi! This looks like a pretty minor migration but I have a few comments that I added below that I'll summarize:
- Would love to see some updated testing options to show variants/states that we aren't seeing, this really helps us prevent regressions (or will once we have these changes in our main branch). Key-focus state was one that I mentioned to add, but it's also helpful to run through these manually too (like for instance, a hover test case usually shouldn't have a hover state, key-focus states shouldn't have an additional key focus state when you tab to the component).
- I think by default we should be seeing some rounding on the swatch, although it's really still unclear to me after looking through specs whether or not the no-rounding option should still exist in S2. Some clarification from design on that might be good here, but doesn't need to be a blocker, but in the mean time since all the default swatches in the specs have rounding, ours should too.
- Swatch group spacing for spacious maybe needs adjusting.
I have a couple of other questions after looking through the spec and the PR too:
- Down state is kind of mentioned in the spec, but no tokens are referenced. Does this mean that swatch gets a down state treatment (with the perspective-based transform)? Not a blocker for this PR but we should find out.
- Is that swatch border color (default) correct? I'm seeing it compute to rgba(19, 19, 19, 0.42) but the spec says gray-1000 which should be 0, 0, 0. If we adjust that though is there a difference between that default border and the light border?
I didn't check WHCM on this pass, but will plan to in the future!
components/swatch/index.css
Outdated
&, | ||
&::before, | ||
&::after, | ||
.spectrum-Swatch-fill, | ||
.spectrum-Swatch-fill::before, | ||
&.is-selected .spectrum-Swatch-fill, | ||
&.is-selected .spectrum-Swatch-fill::before { | ||
border-radius: 0; | ||
border-radius: var(--mod-swatch-border-radius, var(--spectrum-corner-radius-75)); |
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.
Is there a reason why this was replaced with partial rounding? The design changelog seems to indicate that the "no rounding" option has been removed for S2, but both the token and desktop specs indicate that it seems to be sticking around. Either way, the default version of the swatch should have this spec'd corner radius 75 rather than corner-radius-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.
There isn't (at least not a good one) I was just confused by the indication that no rounding was gone, partial was staying and what the default should be. I changed none
as a variant and partial
as the default. ✨
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.
Covering the add swatch here is good! After looking at the spec, I'm wondering if there are a few more improvements we could add to be more thorough, including, but maybe not limited to:
- Key focused states
- Looks like hover, active, and keyfocus have color changes for the add button variant, so showing those would be nice
- Possibly disabled icons in different sizes so we know the size of the icon is changing appropriately
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.
Awesome! I just updated the tests and stories. The add section of the testing grid also has active and hoer states.
The disabled icon sizes are shown in the docs by adjusting the sizes (as is the case with the add icon) — the icon size also reacts to the size control in Storybook. ✨
&.is-keyboardFocused, | ||
&:focus-visible { | ||
outline: var(--mod-swatch-focus-indicator-thickness, var(--spectrum-swatch-focus-indicator-thickness)) solid var(--mod-swatch-focus-indicator-color, var(--spectrum-swatch-focus-indicator-color)); | ||
outline-offset: var(--mod-swatch-focus-indicator-gap, var(--spectrum-swatch-focus-indicator-gap)); | ||
} |
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.
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.
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.
Yep, that definitely helps!
I'm still seeing a very slight, slight difference when key focusing the is-keyboard-focused swatch (or toggling :is-focus-visible on and off), the video below will show that if it works for you.
My best guess is that this is happening because we have the outline focus treatment and also the ::after
focus indicator (around L220), we should pick one or the other. Note that the ::after
has a transition on the opacity to make it look a little smoother when it's key-focused, if we use the outline instead we'd probably want to do the same where a transition effect is placed on the opacity of the outline (I'm guessing from playing with it briefly that there might be a distinction between putting a transition on outline vs. putting it on the outline-color).
It would also be nice to make the focus control toggle on/off when we use the keyboard to focus/remove focus! For example, with button.
Screen.Recording.2025-04-28.at.9.22.36.AM.mov
} | ||
|
||
/* Spacious */ | ||
.spectrum-SwatchGroup--spacious { | ||
--mod-swatchgroup-spacing-regular: var(--mod-swatchgroup-spacing-spacious); | ||
--spectrum-swatchgroup-spacing: var(--spectrum-spacing-100); | ||
--mod-swatchgroup-spacing: var(--mod-swatch-group-spacing-spacious, var(--spectrum-swatch-group-spacing-spacious)); |
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.
Looking at the spec, the spacing for spacious appears to use this token for xs and s, but is different (and not specific to swatch group) for m and l, should that be implemented here?
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 added a mediumLarge
variant since the spacing for those two are the same. I did drop compact as that's no longer present in the specs. I'm happy to restore it, but thought it might best be removed.
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.
Sizing is typically not done with these kinds of selectors. I think we should try to stick with the t-shirt sizing conventions and just list the selectors for sizeM and sizeL for the same styles. They might be the same today but that doesn't mean they'll stay that way.
0fcbcb5
to
db0370d
Compare
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.
Looking good! My biggest feedback note is about the sizing for Swatch Group. Danke!
} | ||
|
||
/* Spacious */ | ||
.spectrum-SwatchGroup--spacious { | ||
--mod-swatchgroup-spacing-regular: var(--mod-swatchgroup-spacing-spacious); | ||
--spectrum-swatchgroup-spacing: var(--spectrum-spacing-100); | ||
--mod-swatchgroup-spacing: var(--mod-swatch-group-spacing-spacious, var(--spectrum-swatch-group-spacing-spacious)); |
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.
Sizing is typically not done with these kinds of selectors. I think we should try to stick with the t-shirt sizing conventions and just list the selectors for sizeM and sizeL for the same styles. They might be the same today but that doesn't mean they'll stay that way.
@@ -37,14 +31,18 @@ export default { | |||
imageUrl: { table: { disable: true } }, | |||
isMixedValue: { table: { disable: true } }, | |||
gradient: { table: { disable: true } }, | |||
isKeyboardFocused: { table: { disable: true } }, |
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.
Should we be importing and extending these settings directly from the Swatch story?
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.
Good call! I totally missed that. I've updated the swatchgroup story to account for that. 🥳
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.
8adbddb
to
f585079
Compare
0887905
to
d44dbf8
Compare
d44dbf8
to
f4b2157
Compare
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.
Hiya, leaving another round of suggestions, the biggest issues I'm currently noticing are the doubled key-focus states and the swatch border tokens I mentioned in the comments.
&.is-keyboardFocused, | ||
&:focus-visible { | ||
outline: var(--mod-swatch-focus-indicator-thickness, var(--spectrum-swatch-focus-indicator-thickness)) solid var(--mod-swatch-focus-indicator-color, var(--spectrum-swatch-focus-indicator-color)); | ||
outline-offset: var(--mod-swatch-focus-indicator-gap, var(--spectrum-swatch-focus-indicator-gap)); | ||
} |
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.
Yep, that definitely helps!
I'm still seeing a very slight, slight difference when key focusing the is-keyboard-focused swatch (or toggling :is-focus-visible on and off), the video below will show that if it works for you.
My best guess is that this is happening because we have the outline focus treatment and also the ::after
focus indicator (around L220), we should pick one or the other. Note that the ::after
has a transition on the opacity to make it look a little smoother when it's key-focused, if we use the outline instead we'd probably want to do the same where a transition effect is placed on the opacity of the outline (I'm guessing from playing with it briefly that there might be a distinction between putting a transition on outline vs. putting it on the outline-color).
It would also be nice to make the focus control toggle on/off when we use the keyboard to focus/remove focus! For example, with button.
Screen.Recording.2025-04-28.at.9.22.36.AM.mov
testHeading: "No rounding", | ||
rounding: "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.
Do we still want to test no rounding here? Since it's still in the controls and we still have the .spectrum-Swatch--roundingNone
class.
.spectrum-Swatch--border { | ||
.spectrum-Swatch-fill { | ||
&::before { | ||
box-shadow: inset 0 0 0 var(--spectrum-swatch-border-thickness) var(--mod-swatchgroup-border-color, var(--spectrum-swatchgroup-border-color)); |
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.
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.
(My understanding of swatch border colors is unfolding in real time). I'm realizing now that there are a few border color tokens:
From spectrum-tokens 13.3.0:
swatch-border-color
: gray-1000
swatch-border-opacity
: 42%
swatch-group-border-color
: gray-1000
swatch-group-border-opacity
: 20%
- this doesn't seem to exist in spectrum-tokens
Then in custom tokens (this is light-vars) we also have these:
--spectrum-swatch-border-color: rgb(0 0 0 / 51%);
--spectrum-swatch-border-color-light: rgb(0 0 0 / 42%);
/**
* `--spectrum-swatch-group-border-color` / `--spectrum-swatch-group-border-opacity`
*/
--spectrum-swatchgroup-border-color: rgb(255 255 255 / 20%);
And then as far as border colors, I'm aware that we previously had light colored borders for certain circumstances, I'm wondering if that's why we're seeing an option for 42% opacity and one for 20% opacity? Maybe? I was thinking before that maybe we ought to remove the light border variant, but now I'm on the fence. Either way, we should clean up the custom tokens to avoid some redundancy between the custom tokens and the tokens that we should have available already.
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.
@rise-erpelding @cdransf Sorry to pop in here, but here's the PR where I left most of the discussion I had with Miruna about the light border stuff.
If I'm understanding Miruna's message correctly, the 42% border opacity (swatch-border-color
) is for single swatches in S2, and then the 20% is for swatches in a swatch group in S2. I don't get the impression that design wants a "light border" option any longer, since Miruna also says: "we decided that for S2 we are gonna be more restrictive with this and give the guidance to have a border for single swatch or swatch group." Re-reading this, I understand her to say design wants all swatches in S2 to have a border, regardless of contrast (I think that's why the light border option existed, even though it was added inconsistently all over the place in a one-off fashion).
I have not reviewed this PR yet, but my instinct is that --spectrum-swatch-border-color (which has the S1 51% opacity in the snippet above) should actually be the 42% opacity for S2. The --spectrum-swatch-border-color-light custom variable (created for the light border option) can be removed. Something more along these lines:
--spectrum-swatch-border-color: rgba(0 0 0 / 42%);
--sspectrum-swatchgroup-border-color: rgba(255 255 255 / 20%)
It's super confusing, so I'm happy to chat if I can clear anything up. We worked a lot out in that PR! ☝️
@@ -37,14 +31,18 @@ export default { | |||
imageUrl: { table: { disable: true } }, | |||
isMixedValue: { table: { disable: true } }, | |||
gradient: { table: { disable: true } }, | |||
isKeyboardFocused: { table: { disable: true } }, |
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.
--spectrum-swatch-border-color: rgb(0 0 0 / 51%); | ||
--spectrum-swatch-border-color-light: rgb(0 0 0 / 20%); | ||
--spectrum-swatch-border-color-light: rgb(0 0 0 / 42%); |
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.
Oh, is this where the light border token is coming from?
Also, do we still need --spectrum-swatch-border-color
? Looks like it's now defined in tokens.
/** | ||
* `--spectrum-swatch-group-border-color` / `--spectrum-swatch-group-border-opacity` | ||
*/ | ||
--spectrum-swatchgroup-border-color: rgb(255 255 255 / 20%); |
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.
It also looks like there's a swatch-group-border-color
token that would be potentially available (but I didn't see swatch-group-border-opacity
, would it be preferable to use the existing swatch-group-border-color-token
and define a custom opacity token instead?
testHeading: "Add", | ||
isAddSwatch: true, |
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.
This coverage looks great!
One thing I noticed since I looked at both forced-colors and WHCM this time around is that we don't have a lot of contrast between the swatch and the icon (.spectrum-Swatch-icon
), while I think we can make improvements to high contrast when we have a more defined spec for it, it would be nice to at least ensure that the contrast is decent for now on these icon swatches.
Here is (I think) forced colors above and WHCM below, you can see that the mixed value and add variants make it really difficult to see the icon here:
…lt; add missing isAddSwatch arg for swatchgroup story
Co-authored-by: [ Cassondra ] <[email protected]>
f4b2157
to
bc1dfdb
Compare
Description
CSS-1026
+CSS-1029
This migrates the
swatch
andswatchgroup
components to S2. Custom properties have been remapped and added per the design specification.An
Add Swatch
variant has been added with the required color tokens and the specified add UI icon.Removed custom properties
--spectrum-swatch-border-color
--spectrum-swatch-dash-icon-color
New custom properties
--spectrum-swatch-border-color-rgb
--spectrum-swatch-border-opacity
--spectrum-corner-radius-full
--spectrum-corner-radius-none
--spectrum-swatch-disabled-icon-border-opacity
--spectrum-swatch-icon-color
--spectrum-swatch-rectangle-width-multiplier
New mods
--mod-add-button-background
--mod-add-button-background-down
--mod-add-button-background-hover
--mod-add-button-background-keyboard-focus
--mod-corner-radius-full
--mod-mixed-button-background
--mod-swatch-border-color-rgb
--mod-swatch-border-opacity
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
Regression testing
Validate:
Screenshots
To-do list