feat(swatch+swatchgroup): S2 migration #3677
Conversation
cdransf
added
wip
🦋 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. |
cdransf
force-pushed
the
cdransf/s2-swatch-group-migration
branch
from
ffa2cad to
37bccbb
Compare
cdransf
force-pushed
the
cdransf/s2-swatch-group-migration
branch
3 times, most recently
from
91c31d2 to
d0944eb
Compare
cdransf
added
size-3 M
cdransf
requested review from
castastrophe,
marissahuysentruyt,
aramos-adobe and
5t3ph
cdransf
force-pushed
the
cdransf/s2-swatch-group-migration
branch
from
9ca18cb to
f3f8289
Compare
cdransf
force-pushed
the
cdransf/s2-swatch-group-migration
branch
2 times, most recently
from
02c2cca to
044c357
Compare
cdransf
added
size-8 XL
There was a problem hiding this comment.
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!
cdransf
force-pushed
the
cdransf/s2-swatch-group-migration
branch
from
0fcbcb5 to
db0370d
Compare
cdransf
force-pushed
the
cdransf/s2-swatch-group-migration
branch
from
8adbddb to
f585079
Compare
…lt; add missing isAddSwatch arg for swatchgroup story
Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com>
…tokens when available
- updates some documentation - removes the light border variants - removes the border control - updates some of the test cases - updates metadata
cdransf
force-pushed
the
cdransf/s2-swatch-group-migration
branch
from
131e6f1 to
a39b0a3
Compare
There was a problem hiding this comment.
Ok, this is getting closer! I think swatch is looking pretty nice aside from some minor things, and I had some questions about the spacing in swatch group.
A couple of things that I noticed that aren't from the migration work here and therefore I couldn't single them out in the code:
- I started playing with the empty/nothing variant a little more. It's nice that you can see it in Storybook by entering
transparentfor the color! This is entirely optional in my opinion, but it'd be nice to add that to theswatchColorcontrol description, something like "Supports standard color input or any valid input for thebackgroundproperty such aslinear-gradient(red, blue)ortransparent." Not sure if there are any other fun swatches you can access just from changingswatchColorto something unexpected that isn't already covered. Just a thought, really up to you! - Another thing I noticed relating to the empty/nothing variant is that our slash goes from top left to bottom right whereas the spec's goes from top right to bottom left. Should we update that here? Playing with it in the browser, I think we've just got the rotation a bit off, but it looked like changing the rotation for the rectangular variant results in the slash ending before it's filled the whole swatch. That one might need an adjustment to the
block-sizeas well?
| border-color: var(--spectrum-swatch-focus-indicator-color); | ||
| border-radius: var(--spectrum-swatch-focus-indicator-border-radius); | ||
|
|
||
| transition: opacity var(--mod-animation-duration-100, var(--spectrum-animation-duration-100)) ease-in-out; |
There was a problem hiding this comment.
This is lovely!! Nice work!
| { | ||
| testHeading: "Empty", | ||
| swatchColor: undefined, | ||
| imageUrl: undefined, | ||
| isMixedValue: false, | ||
| borderStyle: "default", | ||
| isAddSwatch: false, |
There was a problem hiding this comment.
Since we're treating this as a state, the empty + selected and empty + keyfocused combos from the spec are not getting tested:
I'm also wondering if we should have empty swatches in the mixed and add variants. Since we have isMixedValue: false and isAddSwatch: false here they end up getting canceled out. Seems like the appropriate behavior would be that mixed value or add swatches override empty swatches (which they do when I play with the Storybook controls!).
I'm not sure what the right answer is. I like the idea of keeping empty as a state, but maybe refining this test case with ignore: to take out the mixed value and add swatches. And then perhaps we could also add empty into the testData array but use ignore/include to eliminate some of the unneeded states? (At least for sure we don't need empty x empty.)
There was a problem hiding this comment.
Hmm, I added empty to testData and then removed empty, add and mixed value from it in the state data section. I think this makes more sense as it gives us more coverage for the empty states and removes the cases where add/mixed value override empty. ✨
| testHeading: "Extra small", | ||
| density: "sizeXS", | ||
| }, | ||
| { | ||
| testHeading: "Spacious", | ||
| density: "spacious", | ||
| testHeading: "Small", | ||
| density: "sizeS", | ||
| }, | ||
| { | ||
| testHeading: "Large", | ||
| density: "sizeL", |
There was a problem hiding this comment.
I think the "Extra small", "Small", and "Large" here describe the density, right? Can we clarify that in the test heading so as not to be confused with the swatch size?
But looking into it further, I'm a bit confused about the density options, it looks like from the spec the spacing options are compact (same spacing for all swatch sizes), regular (same spacing for all swatch sizes), and spacious (these have different tokens for different sizes)?
I'm not sure we've ever applied tokens depending on the size on a child component, but if I'm understanding correctly that that's what we need to do here, I wonder if this could be a job for :has()?
There was a problem hiding this comment.
Alrighty, I took a look at these changes in e228979. --spectrum-spacing-75 is set as the default (medium variation which is typically our default), then we have sizeS and sizeL where sizeL leverages :has to apply the appropriate density spacing based on the child swatch sizing class.
I split out tests with more informative headers for each density + sizing permutation. ✨
| borderStyle: { | ||
| ...Swatch.argTypes.borderStyle, | ||
| defaultValue: "noBorder", | ||
| description: "Apply the `spectrum-Swatch--lightBorder` class to a swatch in the swatch group when it has a color contrast ratio of less than 3:1.", | ||
| table: { | ||
| type: { summary: "string", required: true }, | ||
| category: "Component", | ||
| defaultValue: { summary: "noBorder" }, | ||
| }, | ||
| options: ["noBorder", "lightBorder"], | ||
| }, |
There was a problem hiding this comment.
Did we decide we were going to eliminate border options completely for swatch and swatch group? There's only one type of border for swatch, a different type of border for swatch group, and then the no-border option is completely gone?
There's a lingering borderStyle arg in the swatch group controls here and also in both templates, we'll have to adjust/remove accordingly.
There was a problem hiding this comment.
I believe so — @marissahuysentruyt may understand it better. But my understanding is that the individual swatches have border style options but those are superseded by the swatchgroup border styles. 😵💫
I went ahead and removed that borderStyle arg in keeping with that understanding. ✨
| --spectrum-swatchgroup-spacing: var(--spectrum-spacing-100); | ||
|
|
||
| /* Border opacity (swatches in a swatch group have a 20% opacity border instead of 42% default border opacity) */ | ||
| --spectrum-swatch-border-opacity: 0.2; |
There was a problem hiding this comment.
We'll probably want to add a comment here to keep Stylelint happy.
…er-opacity in swatchgroup
…erly implement group density
…le degree and sizing
…ixed value states from empty test group
Description
CSS-1026+CSS-1029This migrates the
swatchandswatchgroupcomponents to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification.An
Add Swatchvariant has been added with the required color tokens and the specified add UI icon.The
Add SwatchandMixed Valuevariants may not be combined with background colors or images.The border should not be set when the swatch is selected as the border conflicts with the display of the selected state.
Removed custom properties
--spectrum-swatch-border-color--spectrum-swatch-dash-icon-color(replaced by--spectrum-swatch-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--spectrum-swatchgroup-border-colorNew 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-swatchgroup-border-color--mod-swatch-border-color-rgb--mod-swatch-border-opacityHow 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