feat(tag): migrate to Spectrum 2 #2649
Conversation
|
🚀 Deployed on https://pr-2649--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.52 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailstag
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
2 times, most recently
from
e51d226
to
25f08ea
Compare
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
from
25f08ea
to
9ef3bbe
Compare
components/tag/index.css
Outdated
| /* Passthrough for nested component(s) */ | ||
| --mod-avatar-inline-size: var(--mod-tag-avatar-size, var(--spectrum-tag-avatar-size)); | ||
| --mod-avatar-block-size: var(--mod-tag-avatar-size, var(--spectrum-tag-avatar-size)); | ||
| --mod-thumbnail-size: var(--mod-tag-thumbnail-size, var(--spectrum-tag-thumbnail-size)); |
There was a problem hiding this comment.
These three are following the same approach used for Thumbnails in Table to scale the size of the thumbnail or avatar in the CSS. It looks like another previously used option was to control the sizing in the component such as with Tree View although it seems like this approach would make it easier for scaling to be forgotten.
| @@ -130,26 +129,233 @@ export default { | |||
| type: "migrated", | |||
| }, | |||
| }, | |||
| decorators: [ | |||
There was a problem hiding this comment.
These changes for the Chromatic coverage are modeled after how Button and Action Button are displaying variants in different sections.
There was a problem hiding this comment.
This is looking really good Rise! The Chromatic coverage and Storybook control updates are awesome 👏 Just a couple things:
-
It might be helpful for SWC if we provide them a list of changed/added mods in the docs
-
It looks like Avatar dims when the tag is disabled, which is a carryover from S1. I'm wondering if Thumbnail should behave similarly. Just something to check on during design review
-
Let's also make sure we check on the Invalid disabled state in design review 🤔 I'm wondering if this should look like the other disabled states:
-
WHCM is looking great 👏 The only one that is questionable is Invalid Default when dark mode is turned on. I wonder if we could make this look more like what we have in prod now when forced colors is on?
| content: [ | ||
| { | ||
| xxs: "Extra-extra-small", | ||
| xs: "Extra-small", | ||
| s: "Small", | ||
| m: "Medium", | ||
| l: "Large", | ||
| xl: "Extra-large", | ||
| xxl: "Extra-extra-large", | ||
| }[size], |
There was a problem hiding this comment.
Do we need all these sizes, or just small, medium, and large? 🤔
There was a problem hiding this comment.
I copied this from button/action button, which also don't have as wide a spread of sizes as there are options listed here, and wondered the same thing. I tend to agree and would err on the side of removing the extra sizes unless there's a compelling reason to keep them--I'll plan to take them out.
castastrophe
force-pushed
the
spectrum-two
branch
from
4d9e185
to
a81c712
Compare
There was a problem hiding this comment.
Excellent work on this! This is really close and I only noticed a few small things.
I noticed in the separate Foundations and components changelog that it said "Updated label weight to medium", though I didn't see that listed in changed tokens on the spec. Does the font-weight need to change from regular to --spectrum-medium-font-weight?
I noticed the same thing as Melissa on the invalid + selected + disabled. This issue exists on prod too, so it's not specific to your PR. Disabled styling should likely take precedence there.
The only other thing I was wondering about was the foundations changelog item that said "Should have [...] new down state scale tokens". I'm not sure what design was expecting with that---are they expecting it to scale on click when the close button is clicked and is there something we need to do to support that for SWC? That may be something to bring up in design validation.
| visual: { | ||
| name: "Visual", | ||
| type: { name: "string" }, | ||
| table: { | ||
| type: { summary: "string" }, | ||
| category: "Component", | ||
| }, | ||
| options: [ | ||
| "None", | ||
| "Avatar", | ||
| "Icon", | ||
| "Thumbnail", | ||
| ], | ||
| control: "select", |
There was a problem hiding this comment.
Nice work combining these into a single control!
It might be worth also adding a description to this, like "The type of visual element displayed before the label".
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
from
bdd990d
to
b63ddb9
Compare
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
from
b63ddb9
to
18c75c9
Compare
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
3 times, most recently
from
d746d04
to
54e23c4
Compare
|
Thanks for the feedback! Here's a list of things that have been fixed since being reviewed and a list of outstanding questions: Fixes made:
Questions for design:
|
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
3 times, most recently
from
77c664d
to
e987a9d
Compare
components/tag/index.css
Outdated
| @@ -242,7 +262,8 @@ governing permissions and limitations under the License. | |||
|
|
|||
| /* use negative margin to cancel label to edge margin, then add clear button start margin */ | |||
| margin-inline-start: calc(var(--mod-tag-label-to-clear-icon, var(--spectrum-tag-label-to-clear-icon)) | |||
| - (var(--mod-tag-label-spacing-inline-end, var(--spectrum-tag-label-spacing-inline-end)))); | |||
| - calc(var(--mod-tag-label-spacing-inline-end, var(--spectrum-tag-label-spacing-inline-end)) | |||
| - var(--mod-tag-border-width, var(--spectrum-tag-border-width)))); | |||
There was a problem hiding this comment.
Since the label to edge margin had 1px removed to account for the border-width, this is subtracted here... this is a little complicated, and operationally turns into label-to-clear-icon - label-spacing-inline-end + -border-width. I think it's maybe slightly easier to reason through with a calc() inside the calc() but we definitely can do it another way if that seems easier.
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
f3dd9ff
to
9931a8e
Compare
|
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
from
cd3b43a
to
63683bd
Compare
castastrophe
force-pushed
the
spectrum-two
branch
5 times, most recently
from
6dbc850
to
ae7aedd
Compare
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
from
63683bd
to
62ff0bc
Compare
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
e0e0fd2
to
698e904
Compare
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
from
62ff0bc
to
5d9afa3
Compare
- Remove Express and Spectrum themes
- Use size M properties as default
- Add tokens for S2, including:
- corner rounding (corner-radius-medium-size-<Tshirt size>)
- minimum width (tag-minimum-width-<Tshirt size>)
- maximum width (tag-maximum-width-multiplier)
- clear button spacing (tag-label-to-clear-icon-<Tshirt size>,
tag-edge-to-clear-icon-<Tshirt size>)
- Adds CJK line height
- Add or adjust background colors for variants
- Remove borders (makes them transparent except for WHCM)
- Update font-weight to be medium rather than regular
- Scale avatar sizing
- Support thumbnail within Tag
- Simplify switch options by adding select menu for the visual type
(Avatar, Icon, or Thumbnail) displayed on Tag
- Remove Avatar and Icon stories from Storybook
- Add Chromatic coverage showing multiple variants on one screen
- also allows clear button start spacing to be set to its true value instead of a value offsetting the label spacing and border
- removes disabled state from invalid variants - removes avatar/icon/thumbnail from invalid variants - also removes selected & disabled variant in testing preview - "disabled" is no longer a control when invalid is true
rise-erpelding
force-pushed
the
rise-erpelding/css-618-migrate-tag
branch
from
5d9afa3
to
fb7d0e3
Compare
Description
Migrates the Tag component to Spectrum 2, using new and updated tokens, removing borders, and adding the ability to embed the Thumbnail component.
Express and Spectrum themes removed
Express and Spectrum themes in
express.cssandspectrum.cssrespectively have been removed, and relevant styles moved into theindex.cssfile.Medium is default size
All
sizeMstyles are now the default.New tokens
New tokens for Spectrum 2, including
Color and border changes
Thumbnail added
Tag with Thumbnail is now a visual that is supported (along with Avatar and Icon).
Visuals scale with size change
Small, medium, and large Tags each have appropriately sized icons, avatars, or thumbnails which scale as the component changes size.
Spacing Adjustments
Spacing has been adjusted to match design spec for S2:
Tag with label only
The tag (
.spectrum-Tag) controls the inline-start spacing, while the label (.spectrum-itemLabel) controls the inline-end spacing. Border width is subtracted when setting the margin/padding. Note that.spectrum-Taguses inline-start padding because it’sinline-flex.Tag with label and clear button
The correct clear button inline-start and inline-end spacing are applied to
.spectrum-Tag-clearButton, but since the spacing from the label is still being applied (which has the border width subtracted), this is subtracted from the margin-inline-start being added. This results in1pxof margin being added to the start of the clear button for M and L variants, because the margin being applied to the label is1pxsmaller to account for border width.Tag with Visual (Icon, Avatar, Thumbnail)
These all use the same spacing tokens (
component-edge-to-visual,component-text-to-visual) but there are individual custom props for each visual type to try to avoid issues with migration. Inline-start margin on the visual is offset negatively to account for the difference between the tag padding and what the edge-to-visual value should be. Inline-end spacing remains the same as before.CSS-618
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
Validate Spacing at each size:
Regression testing
Validate:
Screenshots
To-do list