feat(taggroup): spectrum 2 migration #3966
Conversation
🦋 Changeset detectedLatest commit: 1590852 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
File metricsSummaryTotal size: 1.43 MB*
taggroup
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3966--spectrum-css.netlify.app |
f353caa to
ff4d3d4
Compare
| @@ -23,31 +25,78 @@ export default { | |||
| else value.table = { ...value.table, category: "Tag settings" }; | |||
There was a problem hiding this comment.
The tag settings include a disabled control. Do you think we should include any documentation anywhere on "not disabling entire tag groups?" I found a section about that, but I'm not sure if that's something we would want in tag group (like a story, although it feels sort of weird to showcase an entire story just to say "don't do this,") or if it should be with the disabled tag story (that already exists).
There was a problem hiding this comment.
Sure, that's a good callout, I can add this!
Am I correct in interpreting this to mean that while we shouldn't disable all the tags, it's ok to disable some tags? Since I don't think we usually show non-examples in our Storybook, I decided to show an example with 1 tag disabled. Are we ok with this?
Also, that 1 disabled tag template currently is defined in the test file along with all the others, but I don't know that it's worthwhile to add a test for this, so it's defined in the test template and only used in the documentation. I think I like the idea of keeping it there, since the alternatives would be moving some of the templating to the stories file (which we worked to get away from for awhile), or moving it back to the template. I kind of like the idea of a cleaner template, but let me know what you think!
There was a problem hiding this comment.
@rise-erpelding, playing around with the disabled state in Figma I see that the disabled state affects the action button labeled "Show all". So maybe the disabled is only action button specific
There was a problem hiding this comment.
@rise-erpelding yeah, I would interpret it the same as you- we should disable all the tags at once in a tag group, but it's ok to disable some tags. I like the story example, and I think the disabled template you mentioned is fine in the test file, too.
📚 Branch previewPR #3966 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-3966/index.html. |
09f92d9 to
b820eff
Compare
components/taggroup/index.css
Outdated
|
|
||
| /* passthroughs for body typography element in empty state */ | ||
| --mod-body-line-height: var(--spectrum-line-height-100); | ||
| --mod-body-font-size: var(--spectrum-font-size-100); | ||
| --mod-body-margin-start: var(--spectrum-component-top-to-text-100); | ||
| --mod-body-margin-end: var(--spectrum-component-bottom-to-text-100); | ||
|
|
||
| &:lang(ja), | ||
| &:lang(zh), | ||
| &:lang(ko) { | ||
| --mod-body-cjk-line-height: var(--spectrum-cjk-line-height-100); | ||
| } |
There was a problem hiding this comment.
I asked about empty state in the implementations channel, and my understanding from the responses there is that there is no design guidance for empty state, but React still supports it because it's a little weird to have nothing there if there are no tags.
Following suit seems to make sense here and I couldn't get their storybook to cooperate to see what it would look like with no tags & side label, but I'd guess they have fewer layout issues because the empty state font is the same as the field label.
I used the passthroughs on the typography body to adjust the font size and line height to match field label, and also add the same spacing as seen there. That lines it up with field label so that it looks a bit nicer with the side label variant:
Open to thoughts on this though. I can remove the passthroughs if they feel too heavy-handed.
b820eff to
e01bb83
Compare
| /** | ||
| * Avoid disabling an entire tag group. In cases where users can't interact with an entire group of tags, consider either using non-removable tags or hiding the tag group altogether. Don't disable all individual tags; having a tag group that's disabled isn't accessible and it can be frustrating for users. | ||
| */ | ||
| export const Disabled = TagGroupDisabledItem.bind({}); |
There was a problem hiding this comment.
There's an example for disabling the action button as well in Figma. It wasn't documented in the specs but it's configurable prop. I have an example here
| }, | ||
| control: "text", | ||
| }, | ||
| hasDisabledActionButton: { |
There was a problem hiding this comment.
There's now a hasDisabledActionButton control and a new category for action button settings. We could do more with the action button, let me know if you think that's valuable.
There was a problem hiding this comment.
I don't think it's necessary to put more controls for the action button in here- we have a really thorough action button docs page that we linked to already, so I think this is plenty as is!
We could, however, add if: { arg: "actionButtonText", truthy: true }, so that this arg only shows up when the action button renders/has label text.
| /** | ||
| * Avoid disabling an entire tag group. In cases where users can't interact with an entire group of tags, consider either using non-removable tags or hiding the tag group altogether. Don't disable all individual tags; having a tag group that's disabled isn't accessible and it can be frustrating for users. | ||
| * | ||
| * Individual tags may be disabled, and the action button may also be disabled, as seen below. |
There was a problem hiding this comment.
I've also updated this story to include one disabled tag and a disabled action button. I did keep this out of the tests, I'm not sure how valuable it is to show disabled states here when they're already covered within the respective tests for tag and action button, but open to hearing other arguments!
There was a problem hiding this comment.
I don't really think we need to capture them in the tag group tests- like you said, disabled states has already been covered in tag and action button themselves.
| @@ -23,31 +25,78 @@ export default { | |||
| else value.table = { ...value.table, category: "Tag settings" }; | |||
There was a problem hiding this comment.
@rise-erpelding yeah, I would interpret it the same as you- we should disable all the tags at once in a tag group, but it's ok to disable some tags. I like the story example, and I think the disabled template you mentioned is fine in the test file, too.
| }, | ||
| control: "text", | ||
| }, | ||
| hasDisabledActionButton: { |
There was a problem hiding this comment.
I don't think it's necessary to put more controls for the action button in here- we have a really thorough action button docs page that we linked to already, so I think this is plenty as is!
We could, however, add if: { arg: "actionButtonText", truthy: true }, so that this arg only shows up when the action button renders/has label text.
| isRemovable: true, | ||
| isEmphasized: false, | ||
| customStyles: {"max-width": "300px"}, |
There was a problem hiding this comment.
I don't think this is blocking anything, but do we want to update this to max-inline-size at all? Should we also mention that "implementations can set a max inline size on .spectrum-TagGroup to facilitate wrapping?"
I don't think either are technically blocking.
Description
The Spectrum 2 version of Tag Group is a major change from its Spectrum 1 counterpart.
Style changes
In order to support the aforementioned spacing changes, the two mod properties for margin have been removed:
--mod-tag-group-item-margin-block--mod-tag-group-item-margin-inlineInstead, please customize spacing between tags with:
--mod-tag-group-block-tag-spacing--mod-tag-group-inline-tag-spacingThese custom properties may need to be set to be double the previous margin values in order to achieve the same spacing.
To support custom spacing of the embedded components, several other new mod properties have been added:
--mod-tag-group-block-spacing-label-to-tags--mod-tag-group-inline-spacing-label-to-tags--mod-tag-group-spacing-help-text-to-tagsTo support the optional empty state (when there are no tags in the tag group), several passthroughs to modify the body typography text element have been added, including:
--mod-body-cjk-line-height--mod-body-font-size--mod-body-line-height--mod-body-margin-end--mod-body-margin-startTemplate and Storybook changes
In addition to styling changes, several additions to the markup have been made here, and to better support these changes, Storybook controls have also been updated:
Because of the drastic layout and feature changes in this component migration, the testing has also changed. The testing grid continues to include removable tags, sizing, and tags that wrap to multiple lines, but also shows the other embedded components that have been added. Empty state and side label variants are also added as test cases.
Questions/things I'd love feedback on
CSS-1037
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