-
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!
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!
8adbddb
to
f585079
Compare
…lt; add missing isAddSwatch arg for swatchgroup story
Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com>
…tokens when available
ad29336
to
120625c
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.
@cdransf Could we add tags: ["migrated"]
to swatch group & swatch? I noticed some of our migrated components have that, and some don't, but I think that'd be a nice thing to do!
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