feat(tag): migrate to S2 #3682

rise-erpelding · 2025-04-22T21:33:53Z

Description

There was a previous version of this work in #2649 that had been shelved because icons had not been updated yet. That PR will be closed once this one merges.

This work migrates the tag component to Spectrum 2 tokens and specifications:

The invalid variant has been removed.
Borders remain in high contrast but are otherwise transparent.
Thumbnail has been added as a visual option.
Tag now has a max-inline-size.
Read-only tags are now supported.

Additionally, Tag has an expanded testing grid to test more states and state combinations, an updated Docs page in Storybook, and updated Storybook controls to demonstrate Tag's supported content and states.

There are a few outstanding questions that I have, but I don't consider these to be blocking and will make follow-up tickets as needed to address these items:

Down state seems to behave erratically, but this appears to happen with other components too, navigating to and from the testing grid appears to affect this (we now have a ticket for this).
We don't have WHCM specs for S2, so the styles here are best-guess. I did end up leaving the clear button colors as they were in S1 for now.
The neutral-selected-background-color-default and its accompanying hover, key-focus, down tokens don't appear to exist, I used --spectrum-neutral-background-color-default colors, which seem to mostly match the spec colors.
I saw that the S1 guidelines mentioned a read-only Tag, but this doesn't appear to have been implemented. This is pretty minor to support, it won't affect implementations if they don't use it, and could be removed easily. I'll follow up on whether or not we want this and if there are any additional guidelines on what it should look like. For now, read-only tags do not change if selected, hovered, focused, or active, but disabled does win out over read-only. The text is selectable now in all states (including disabled).
Emphasized tags - there's no clarification on what emphasized & selected vs. emphasized & unselected should look like, my guess is that the tag only appears emphasized when it's selected. This is not how it's implemented in S1, but it was easy to change and would be easy to change back if that's not correct.
Clear button - there aren't clear guidelines on how this should behave, should it change color on hover or have a focus outline? The only design guideline is about which icon to use.
We changed the avatar size for the medium tag to size-75, this is what React appears to be doing and it fits the tag better, is this acceptable or do we want to have the avatar size-100 as spec'd?
The default t-shirt size has been changed to small per design note, but might be good to clarify.

Questions I have now:

I made the text selectable, but should I make it unselectable for the disabled state?

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

Here are some things that I generally look at when reviewing other people's migrations that could be used to review this migration, plus a few specific callouts for this work that I'd love feedback on:

Check against token spec:
- Validate that all tokens used appropriately/as spec'd?
- Specifically, does the spacing match the spec?
Check through changed files:
- Nothing out of scope appears to have been changed
- Double-check for typos or anything else that feels inaccurate or illogical
Check Windows High Contrast Mode (I noted that this looked different in AssistivLabs and forced-colors rendering in Chrome, it's not a bad idea to check both if you have the bandwidth):
- Colors are high contrast and make sense given the way other components are often styled
Check the Docs page:
- No typos, information covers the changes appropriately
- I left documentation about the removal of the invalid tag out, but could add it, open to opinions here!
Check the Storybook controls:
- They work and cover all variants, states and combinations thereof
Check the testing grid:
- Is there sufficient test coverage? Are there any states that aren't covered? Any ideas for organizing this better?
- Specifically, I didn't cover focus + hover, is there any need to for Tag?
View read-only state. I'll likely start a Slack conversation about this, but I think this work can be reviewed without knowing the answer:
- Read-only behaves as expected - they appear as normal tags, but you can't interact with them, disabled should always take precedence if a tag is read-only and disabled.
Clear button
- Does the clear button behave as expected?

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

Screenshots

Spectrum 2:

Spectrum 1:

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2025-04-22T21:33:56Z

🦋 Changeset detected

Latest commit: a0495d5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/tag	Major

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

github-actions · 2025-04-22T21:38:53Z

File metrics

Summary

Total size: 1.37 MB^*

Package	Size	Minified	Gzipped
tag	21.35 KB	20.40 KB	2.89 KB

tag

Filename	Head	Minified	Gzipped	Compared to base
index.css	21.35 KB	20.40 KB	2.89 KB	🟢 ⬇ 6.17 KB
metadata.json	11.40 KB	-	-	🟢 ⬇ 2.81 KB

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions · 2025-04-22T21:39:38Z

🚀 Deployed on https://pr-3682--spectrum-css.netlify.app

rise-erpelding · 2025-04-23T16:16:25Z

components/tag/index.css

+
+	/* border color - transparent unless modified or high contrast mode */
+	--spectrum-tag-border-width: var(--spectrum-border-width-100); /* borders remain in WHCM but appear transparent otherwise */
+	--spectrum-tag-border-color: var(--highcontrast-tag-border-color, var(--mod-tag-border-color, transparent));


Colors have var-stacks at the top level, other properties tend to be numeric or change between sizes, so I left those as-is.

components/tag/index.css

rise-erpelding · 2025-04-23T16:20:33Z

components/tag/index.css


-	--spectrum-tag-avatar-spacing-block-start: var(--spectrum-tag-top-to-avatar-medium); /* 6px 9px */
+	--spectrum-tag-avatar-size: var(--spectrum-avatar-size-100);


Avatar and thumbnail size don't correspond to the usual 75 = small, 100 = medium, 200 = large sizing conventions.

Do you think we should confirm with design that a avatar sizes for medium and large tags are the same? Instead of what's in Figma (50, 100, 100), I wonder if the avatar sizes should be 50, 75, 100. Thumbnail and the cross icon have different sizes in the token specs based on the tag's t-shirt size, but avatar didn't, so I just wanted to check.

Oh I think this might be a vote towards double checking the medium tag's size-100 avatar. Is the avatar container, plus the margin that's in the specs supposed to overflow the tag's height?

If I change to size-75 for the avatar in the medium tag, it looks like everything remains contained.

It does fit better with medium and it looked like this avatar size is how React has it, so I changed it and moved it to my list of things to ask about, but to be fair, the avatar's margin is still slightly (2px I think) out of bounds in the large tag:

rise-erpelding · 2025-04-23T16:26:56Z

components/tag/index.css

+		margin-block-start: calc(var(--mod-tag-clear-button-spacing-block, var(--spectrum-tag-clear-button-spacing-block)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));
+		margin-block-end: calc(var(--mod-tag-clear-button-spacing-block, var(--spectrum-tag-clear-button-spacing-block)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));


The padding for block spacing and margin for inline spacing that was here before was making the cross icon's focus outline distorted. It's definitely too tight as it is now. I'm kind of at a loss about what to do with this, open to ideas.

S1:

S2:

Yeah this is a tough one. If I inspect this in Figma, it's not even a Figma component- it's just a rectangle with that icon in it, where typically the components have been built with the other Figma components so they get all of the usual styles...I might come back to this, but right now, this is a tricky one.

Leaving a todo comment for myself, since we talked about this: we'll remove the clear button focus outline/remove the clear button from the tabindex. And it would also be nice to make sure that the target for the clear button is of an accessible size.

Ok, clear button is no longer focusable. I brought the padding back to what it was before (rather than using margin). It looks like there was some onclick event handling that didn't work (it's possible it never worked? I didn't see clear button accepting an onclick arg previously or currently), I did wire that up so that it works as intended but am happy to remove it if we think it's unnecessary. You can't remove a tag on delete as you would be able to on SWC or React, but that feels like it's outside of the scope of what we should be implementing in CSS (the onclick removability is probably outside of the scope by that logic too, which is why I felt weird about fixing it but didn't want to delete it).

I didn't update the spacing on clear button, mainly just because I still have it as a to-do item to update the clear button target to be an accessible size (24px). I kind of think that if we're gonna go and do that, the whole spacing system needs to be refactored. The previous PR #2649 started using :has, which is a nice idea, and seems to be supported in SWC, but I still feel a little uneasy about making that change (I'd really want to know more about whether this causes other issues downstream). I'm open to taking another look within this PR or moving this to follow-up work.

components/tag/index.css

rise-erpelding · 2025-04-23T16:48:43Z

components/tag/stories/tag.stories.js

 	},
+	decorators: [
+		withDownStateDimensionCapture,


I've noticed this sometimes stops working if you switch to the testing grid and back, but this happens with other components too. Something else I'd like to follow up on. It generally works fine if you reload.

I noticed that in other components, too. I feel like I really noticed it in action button, but I just chalked it up to there being so much markup in the testing grid.

There's a ticket for this!

rise-erpelding · 2025-04-23T16:50:14Z

components/tag/stories/tag.stories.js

+/**
+ * Tags have a read-only option for when content in the disabled state still needs to be shown.
+ * Read-only tags cannot be interacted with or changed.
+ */
+export const ReadOnly = TagsDefaultOptions.bind({});
+ReadOnly.storyName = "Read only";
+ReadOnly.tags = ["!dev"];
+ReadOnly.args = {
+	isReadOnly: true,
+};
+ReadOnly.parameters = {
+	chromatic: { disableSnapshot: true },
+};
+


This is a follow-up item also, it wasn't in the design spec but has existed in the S1 design guidelines.

The guidelines also show the label text as being selectable and copyable, but currently it can't be selected and copied. The intended behavior needs some clarification; maybe something else to note for the follow-up.

I removed user-select: none; from the tag, so now the text is selectable (but not copy-able, is there a native way to do that in CSS?). Happy to restore it if that causes unintended issues though.

rise-erpelding · 2025-04-23T16:52:38Z

components/tag/stories/template.js

 			<span class="${rootClass}-itemLabel">${label}</span>
 			${when(hasClearButton, () =>
 				ClearButton({
-					size,


Many of these (clear button, avatar, thumbnail) have their size set by CSS, there doesn't seem to be a lot of value to passing down the size in the template, but let me know if it seems like I'm missing something.

I agree, especially with the clear button. 👍

I was playing around with avatar and thumbnail, and I found this size mapping (?) approach, which I believe works too:

Thumbnail({ size: [{ s: "50", m: "75", : "100", }[size] || "75"], imageURL: thumbnailUrl, }, context) Avatar({ size: [{ s: "50", m: "100", l: "100", }[size] || "100"], image: avatarUrl, }, context)

Is that better than explicitly setting the size in the CSS? I'm not really sure. Maybe it's better to set it with CSS instead of in the template? We could probably get rid of some CSS if we used that ☝️ , but...??? any thoughts on that?

I think both ways achieve the same thing so, I'll leave it up to you which way you think is best!

I think this question about which approach is better was the origin of why we decided that it would be nice to have a style guide (and it was also in relation to this tag migration the first time around). I'll take a look at this too, I think my initial thought is always "but we have a token for it, so we should use it" and it helps ensure that we get it shipped with the right size, but on the other hand, extra unnecessary code isn't always the best thing. I'll take another look at this one too.

Ok, it is size-mapped in the template... but we're also using the tokens. Overkill? It feels like putting in the CSS is nice, because the visuals will be appropriately sized without needing to add it in the implementation, but is there an argument against that?

components/tag/stories/tag.test.js

marissahuysentruyt

Another PR, back from the dead! Thanks for the explanation on the padding/margin stuff with all of the new options- very helpful. I think overall the tokens are in a really good spot- there was only one I didn't see in the CSS (about the down state).

I found some style criss-crossing when I started to really mix up the controls, so I left some ideas on how to expand the test file so we covered more of those.

But I had just one extra thing to add, that I couldn't add to the code...

There's a note about the tooltip on hover to reveal the full text in the docs for text overflow. Is it helpful at all the mention that our component doesn't do that? 😆

marissahuysentruyt · 2025-04-25T13:27:16Z

components/tag/stories/tag.stories.js

-			name: "Clear button",
-			description: "True if a button is present to clear the tag.",
+			name: "Removable",
+			description: "Has a clear button to clear the tag.",


Maybe to lean into the "Removable" name for this control, the description could be "Has a clear button to remove the tag from the tag group" or "Has a clear button to remove the chosen tag."

Just a thought.

Control is now removable instead of clear button!

marissahuysentruyt · 2025-04-25T13:29:41Z

components/tag/stories/tag.stories.js

 	},
+	decorators: [
+		withDownStateDimensionCapture,


I noticed that in other components, too. I feel like I really noticed it in action button, but I just chalked it up to there being so much markup in the testing grid.

marissahuysentruyt · 2025-04-25T13:38:32Z

components/tag/stories/tag.stories.js

-
+/**
+ * Tags should always include a label to represent search terms, filters, or keywords. Tags also
+ * have the option to include an icon, avatar, or thumbnail in addition to the label.


What do you think about adding links to each of these docs pages?

Links added!

marissahuysentruyt · 2025-04-25T13:54:48Z

components/tag/stories/tag.stories.js

+ * Read-only tags cannot be interacted with or changed.
+ */
+export const ReadOnly = TagsDefaultOptions.bind({});
+ReadOnly.storyName = "Read only";


Super tiny thing, but should we have a hyphen here: "Read-only?" I only ask because the description has the hyphens.

Yep, hyphen should be in there!

components/tag/stories/template.js

marissahuysentruyt · 2025-04-25T16:28:14Z

components/tag/index.css

+		color: var(--spectrum-tag-content-color-active);
+
+		/* stylelint-disable-next-line spectrum-tools/no-unknown-custom-properties -- height and width are set by implementations */
+		transform: perspective(max(var(--spectrum-downstate-height), var(--spectrum-downstate-width) * var(--spectrum-component-size-width-ratio-down))) translateZ(var(--spectrum-component-size-difference-down));


I didn't see component-size-minimum-perspective-down token in the CSS. Is that supposed to be added to the smallest size tag, sort of like the extra small and small action buttons use that token?

My understanding based on how this is documented on our Foundations page is that we use minimum perspective for little components, and calculated perspective (which needs the down state decorator) for anything with a width larger than 24px. It's true that it's possible for tag-minimum-width-small to be 21px for desktop, but I believe in that case the height for the small tag would be 24px and so, using the max formula to determine whether the height or width is larger, we'd get the same perspective value as if we'd used the --component-size-minimum-perspective-down token (which is 24px).

My guess is that --component-size-minimum-perspective-down might be included in the spec as sort of a catchall that they add to all components that receive the down state treatment?

marissahuysentruyt · 2025-04-25T16:33:35Z

components/tag/index.css

+		margin-block-start: calc(var(--mod-tag-clear-button-spacing-block, var(--spectrum-tag-clear-button-spacing-block)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));
+		margin-block-end: calc(var(--mod-tag-clear-button-spacing-block, var(--spectrum-tag-clear-button-spacing-block)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));


Yeah this is a tough one. If I inspect this in Figma, it's not even a Figma component- it's just a rectangle with that icon in it, where typically the components have been built with the other Figma components so they get all of the usual styles...I might come back to this, but right now, this is a tricky one.

marissahuysentruyt · 2025-04-25T16:39:54Z

components/tag/index.css

 			inset-block-end: calc(-1 * var(--mod-tag-focus-ring-gap, var(--spectrum-tag-focus-ring-gap)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)) - var(--mod-tag-focus-ring-thickness, var(--spectrum-tag-focus-ring-thickness)));
 			inset-inline-start: calc(-1 * var(--mod-tag-focus-ring-gap, var(--spectrum-tag-focus-ring-gap)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)) - var(--mod-tag-focus-ring-thickness, var(--spectrum-tag-focus-ring-thickness)));
 			inset-inline-end: calc(-1 * var(--mod-tag-focus-ring-gap, var(--spectrum-tag-focus-ring-gap)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)) - var(--mod-tag-focus-ring-thickness, var(--spectrum-tag-focus-ring-thickness)));


Could these 4 inset properties be combined into a single inset? Would there ever be a time where these would change? Or should we keep them just to offer flexibility, if...the focus ring styles change?

Things are working as is, this was just another thought.

marissahuysentruyt · 2025-04-25T16:49:59Z

components/tag/index.css


-	--spectrum-tag-avatar-spacing-block-start: var(--spectrum-tag-top-to-avatar-medium); /* 6px 9px */
+	--spectrum-tag-avatar-size: var(--spectrum-avatar-size-100);


Oh I think this might be a vote towards double checking the medium tag's size-100 avatar. Is the avatar container, plus the margin that's in the specs supposed to overflow the tag's height?

If I change to size-75 for the avatar in the medium tag, it looks like everything remains contained.

marissahuysentruyt · 2025-04-25T17:07:54Z

components/tag/index.css


 	border-radius: var(--mod-tag-corner-radius, var(--spectrum-tag-corner-radius));
 	border-width: var(--mod-tag-border-width, var(--spectrum-tag-border-width));
 	border-style: solid;

-	padding-inline-start: calc(var(--mod-tag-spacing-inline-start, var(--spectrum-tag-spacing-inline-start)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));
+	padding-inline-start: calc(var(--mod-tag-label-spacing-inline, var(--spectrum-tag-label-spacing-inline)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));


Thanks for this explanation- it's pretty wild to get these spacing values correct.

I did notice an inconsistency with the label's margin-inline-end when it's next to the clear button. I think maybe we don't need to subtract the border width? The medium and large spacing between the label text and clear button is off by one if I compare to Figma. Technically, the small spacing checks out correctly, but that's because the token is set to 9px instead of 8px like Figma says.

Unfortnately, I don't really have a solution to offer. I can spend some time and see what I come up with.

jawinn · 2025-04-28T18:23:00Z

components/tag/stories/tag.stories.js

+/**
+ * Tags have a read-only option for when content in the disabled state still needs to be shown.
+ * Read-only tags cannot be interacted with or changed.
+ */
+export const ReadOnly = TagsDefaultOptions.bind({});
+ReadOnly.storyName = "Read only";
+ReadOnly.tags = ["!dev"];
+ReadOnly.args = {
+	isReadOnly: true,
+};
+ReadOnly.parameters = {
+	chromatic: { disableSnapshot: true },
+};
+


The guidelines also show the label text as being selectable and copyable, but currently it can't be selected and copied. The intended behavior needs some clarification; maybe something else to note for the follow-up.

jawinn · 2025-04-28T18:56:45Z

components/tag/stories/tag.test.js

+			isSelected: true,
+		},
+		{
+			testHeading: "Emphasized, selected",


Good callout on this one Marissa. I was wondering the same thing when looking this over. Looking at the desktop file and the playground, Figma only allows turning on emphasized in combination with selected. We don't have updated guidelines but I think you may be right that is only affects the selected state.

In that case, the emphasized Docs entry could be "Default - selected + emphasized" with "The emphasized option uses emphasized colors only when the tag is selected". And tests would need a few updates. Along with styles to make sure just emphasized or emphasized + removable doesn't show emphasized colors.

jawinn · 2025-04-28T19:01:31Z

components/tag/stories/tag.test.js

+		{
+			testHeading: "Emphasized, read-only",
+			isReadOnly: true,
+			isEmphasized: true,
+		},


This one was showing emphasized colors but I would expect it to show read-only colors.

Because we expect emphasized colors to take effect only when it's selected? I think I have this updated properly now:

jawinn · 2025-04-28T19:29:35Z

components/tag/stories/template.js

+			@focusin=${function() {
+				if (isReadOnly || isDisabled) return;
+				updateArgs({ isFocused: true });
+			}}
+			@focusout=${function() {
+				updateArgs({ isFocused: false });
+			}}


I'd recommend removing this, as the focus events are not the same behavior as :focus-visible. So currently if you click on the tag in the Default story, the focus indicator will appear when it shouldn't.

Ok! I'm using the keyboard focused instead of focused control now as well. I think that I'd thought it would be nice to see the control toggling when you keyboard focus, but that does have unintended side effects.

jawinn · 2025-04-28T19:34:05Z

components/tag/stories/tag.stories.js

Docs addition suggestion: I think it's worth mentioning in the Truncated story description that tags have a maximum width set. Since that might not be expected until it was encountered.

This shouldn't have had a maximum width set on the custom styles, since tag already has a max inline size set. I updated this story to show the truncated, max-width tag for all 3 sizes.

jawinn · 2025-04-28T19:39:07Z

components/tag/index.css

-		background-color: var(--highcontrast-tag-background-color-focus, var(--mod-tag-background-color-focus, var(--spectrum-tag-background-color-focus)));
-		color: var(--highcontrast-tag-content-color-focus, var(--mod-tag-content-color-focus, var(--spectrum-tag-content-color-focus)));
+	&:not(.is-disabled, .is-readOnly):focus-visible,
+	&:not(.is-disabled, .is-readOnly).is-focused {


Do we still need .is-focused for S2 if it's already handled by the :focus-visible? Is this a legacy thing? If needed for the implementation to apply, it should probably be documented.

I don't think there's a reason that it needs .is-focused, it's now using :focus-visible only and keyboard focused in the controls.

jawinn · 2025-04-28T19:55:05Z

components/tag/index.css

+	--spectrum-tag-background-color-selected: var(--highcontrast-tag-background-color-selected, var(--mod-tag-background-color-selected, var(--spectrum-neutral-background-color-default)));
+	--spectrum-tag-background-color-selected-hover: var(--highcontrast-tag-background-color-selected-hover, var(--mod-tag-background-color-selected-hover, var(--spectrum-neutral-background-color-hover)));
+	--spectrum-tag-background-color-selected-active: var(--highcontrast-tag-background-color-selected-active, var(--mod-tag-background-color-selected-active, var(--spectrum-neutral-background-color-down)));
+	--spectrum-tag-background-color-selected-focus: var(--highcontrast-tag-background-color-selected-focus, var(--mod-tag-background-color-selected-focus, var(--spectrum-neutral-background-color-key-focus)));


Could "selected" tokens be used for the value of existing custom properties in .spectrum-Tag.is-selected, instead of creating new custom properties? For example:

&.is-selected { --spectrum-tag-background-color: var(--highcontrast-tag-background-color-selected, var(--mod-tag-background-color-selected, var(--spectrum-neutral-background-color-default))); --spectrum-tag-content-color: var(--highcontrast-tag-content-color-selected, var(--mod-tag-content-color-selected, var(--spectrum-gray-25))); --spectrum-tag-border-color: var(--highcontrast-tag-border-color-selected, var(--mod-tag-border-color-selected, transparent)); }

I'll have to come back to this one, I made an attempt but started getting tangled when it came to hover/focus/active states. Is button a good component to reference to see where else it's done like this?

copies CSS tokens and formatting from old PR - Removal of some border-color styles and custom props for specific variant states. These styles are no longer needed as borders have been changed to transparent by default and in WHCM many variants do not change border color between states (hover, active, focus, etc.). - Set border color var-stack at top level - add clear button spacing tokens from previous migration - add min-inline size tokens - clean up passthroughs and consolidate to a single section - remove invalid variant from css - implement down state - adjust avatar and thumbnail sizing - set max inline size - adjust typography tokens - color var stacks up top - nested component cleanup - clear button uses passthroughs - support readonly - whcm updates

- updates to support visual content in storybook template: - icon and avatar controls moved to content section - thumbnail added - add visual control menu to choose visual -remove invalid variant references and options - support active, focus, hover - add documentation - minor refactoring and cleanup - support readonly

- remove selected template - add removable tag to grouped template - prevent showing of duplicate removable tag for in removable story - use grouped template for sizing

ADJUST FOCUS BEHAVIOR: - remove .is-focused states from CSS to use native pseudo-classes - use keyboard focused global state instead of focused global state in Storybook - change of arg from isFocused to isKeyboardFocused in template and tests - removal of focus event listeners to update args ADJUST EMPHASIZED STYLING - .is-emphasized is now .spectrum-Tag--emphasized in CSS and template - emphasized styles only take effect if also selected - update tests and test headings to better display the emphasized variant - small adjustments to story names on docs page to put selected stories next to each other and show emphasized and selected rather than just emphasized ADJUST READ ONLY STYLING - Remove user-select: none to make tag text selectable per guidelines - prevent selected styles from having effect if read-only or disabled - adjust emphasized & read-only tests to include selected state MISC OTHER FIXES - hasClearButton control has been changed to isRemovable - Update of Truncated story to remove max width and rely on component's max width

- change avatar size to 75 for medium - use flex-shrink to prevent thumbnail width shrinking when the label is long - use size map in the template for avatar and thumbnail sizing - fix removable tag functionality - remove focusability on clear button (it doesn't remove on delete but is that functionality that's needed here?), revert margin on clear button back to padding where it's not negative - add default sizing info on sizing story - combine inset focus indicator properties to use shorthand

rise-erpelding · 2025-04-30T20:59:15Z

components/tag/index.css

 	outline: none;
-	user-select: none;


Is there a reason to keep this?

rise-erpelding · 2025-04-30T21:02:56Z

components/tag/index.css

+
+	.spectrum-Thumbnail {
+		/* prevent thumbnail shrinking when label is longer */
+		flex-shrink: 0;


Unclear why only thumbnail seems to shrink when the label is long.

I did think about adding thumbnail/avatar/icon to the truncated section of tests and didn't do it because it would involve moving several things around the test file and the testing grid is already starting to feel a little bloated to me, I'm open to the idea though.

rise-erpelding added wip This is a work in progress, don't judge. S2 Spectrum 2 labels Apr 22, 2025

rise-erpelding force-pushed the rise-erpelding/tag-s2-pt2 branch 2 times, most recently from 937827f to f5cf2f3 Compare April 23, 2025 16:12

rise-erpelding marked this pull request as ready for review April 23, 2025 16:13

rise-erpelding added ready-for-review and removed wip This is a work in progress, don't judge. labels Apr 23, 2025

rise-erpelding commented Apr 23, 2025

View reviewed changes

rise-erpelding mentioned this pull request Apr 23, 2025

feat(tag): migrate to Spectrum 2 #2649

Closed

17 tasks

rise-erpelding commented Apr 24, 2025

View reviewed changes

components/tag/stories/tag.test.js Show resolved Hide resolved

rise-erpelding self-assigned this Apr 24, 2025

marissahuysentruyt requested changes Apr 25, 2025

View reviewed changes

jawinn requested changes Apr 28, 2025

View reviewed changes

rise-erpelding force-pushed the rise-erpelding/tag-s2-pt2 branch from 9741335 to 51c77f3 Compare April 29, 2025 11:12

rise-erpelding added 8 commits April 29, 2025 19:53

test(tag): set up testing grid

f19d711

chore(tag): metadata

8e3227e

chore(tag): changeset

68b7ab0

docs(tag): template adjustments

179c892

- remove selected template - add removable tag to grouped template - prevent showing of duplicate removable tag for in removable story - use grouped template for sizing

fix(tag): make small tag's default size

eec2584

rise-erpelding force-pushed the rise-erpelding/tag-s2-pt2 branch from 51c77f3 to eec2584 Compare April 30, 2025 00:58

rise-erpelding commented Apr 30, 2025

View reviewed changes

components/tag/index.css

outline: none;

user-select: none;

Copy link

Collaborator Author

rise-erpelding Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to keep this?

rise-erpelding commented Apr 30, 2025

View reviewed changes

castastrophe force-pushed the spectrum-two branch from 2de9dee to ef31bff Compare May 5, 2025 20:42

castastrophe force-pushed the spectrum-two branch 2 times, most recently from 7e8269c to 4ce8d19 Compare May 5, 2025 21:18


		--spectrum-tag-avatar-spacing-block-start: var(--spectrum-tag-top-to-avatar-medium); /* 6px 9px */
		--spectrum-tag-avatar-size: var(--spectrum-avatar-size-100);

		margin-block-start: calc(var(--mod-tag-clear-button-spacing-block, var(--spectrum-tag-clear-button-spacing-block)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));
		margin-block-end: calc(var(--mod-tag-clear-button-spacing-block, var(--spectrum-tag-clear-button-spacing-block)) - var(--mod-tag-border-width, var(--spectrum-tag-border-width)));

feat(tag): migrate to S2 #3682

Are you sure you want to change the base?

feat(tag): migrate to S2 #3682

Conversation

rise-erpelding commented Apr 22, 2025 • edited Loading

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

changeset-bot bot commented Apr 22, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Apr 22, 2025 • edited Loading

File metrics

Summary

tag

github-actions bot commented Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marissahuysentruyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawinn Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rise-erpelding commented Apr 22, 2025 •

edited

Loading

changeset-bot bot commented Apr 22, 2025 •

edited

Loading

github-actions bot commented Apr 22, 2025 •

edited

Loading

github-actions bot commented Apr 22, 2025 •

edited

Loading

jawinn Apr 28, 2025 •

edited

Loading