feat(taggroup): spectrum 2 migration #3966
Conversation
🦋 Changeset detectedLatest commit: ff4d3d4 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.42 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsaccordion
contextualhelp
taggroup
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3966--spectrum-css.netlify.app |
rise-erpelding
added
the
size-3 M
rise-erpelding
force-pushed
the
rise-erpelding/css-1037-s2-tag-group
branch
from
f353caa to
ff4d3d4
Compare
rise-erpelding
added
ready-for-review
and removed
wip
marissahuysentruyt
added
the
skip_vrt
| }, context)} | ||
| `)} | ||
| ${when(actionButtonText, () => html` | ||
| ${ActionButton({ |
There was a problem hiding this comment.
To respond to your concerns about the text alignment- I think this is fine, at least for a first pass. The text is aligned like this in Figma, with the field label text right up against the container, but then some gap-age for the tags' and this quite action button's text. The button is lined up with the edge of the container even though we can't see the edge of the button, as are the tags, so to me, it's fine.
I looked at the React tag group link you had too, and their's looks to have the text visually offset so, I don't personally have much of a concern about it.
| helpText: "", | ||
| actionButtonText: "", | ||
| }; | ||
|
|
There was a problem hiding this comment.
What do you think about adding a sizes story? We can mention the default size is medium for tag group (I think that's right).
/**
* Tag groups are available in different sizes to match the size of the tags within them.
*/
export const Sizing = (args, context) => Sizes({
Template,
withHeading: false,
withBorder: false,
...args,
}, context);
Sizing.tags = ["!dev"];
Sizing.parameters = {
chromatic: { disableSnapshot: true },
};
(obviously my quick attempt at docs was super duper generic 😆)
There was a problem hiding this comment.
or no, was TagGroupSizingTemplate meant to be imported here too?
| { label: "Sports" }, | ||
| { label: "Surfing" }, | ||
| { label: "Water" }, | ||
| { label: "Hawaii" }, |
There was a problem hiding this comment.
It's almost like Hawaii was on your mind!!!!
| - `--mod-tag-group-item-margin-block` | ||
| - `--mod-tag-group-item-margin-inline` | ||
|
|
||
| Instead, please customize spacing between tags with: |
There was a problem hiding this comment.
I like the voice you took in the changeset, like instructing what someone would need to do to migrate to this version. I might steal that 😊
| .spectrum-TagGroup--sizeL { | ||
| --spectrum-tag-group-inline-tag-spacing: var(--spectrum-spacing-200); | ||
| --spectrum-tag-group-block-tag-spacing: var(--spectrum-spacing-200); | ||
| } |
There was a problem hiding this comment.
We may not even really need this selector, since it's exactly the same values as the medium tag group. I can, however, see an argument for keeping it anyways, just to make it clear where the large styles live. It follows our preferred t-shirt sizing pattern regardless.
| @@ -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).
| }, | ||
| numberOfTags: { | ||
| name: "Number of tags", | ||
| description: "The number of tags to display in the tag group.", |
There was a problem hiding this comment.
Feel free to disregard, but you could add the blurb like you have for action button & help text. Something like, "If there are no tags, the empty state will render?" I don't know, maybe not...
| }; | ||
|
|
||
| /** | ||
| * A tag group on its own should always have a label. Labels can be placed either on top or on the side on the tags, but top labels are the default and are recommended because they work better with long copy, localization, and responsive layouts. |
There was a problem hiding this comment.
Since this story technically gets removed from the docs page, do we need to have the JSDoc? It looks like it's repeated anyways with the "Label position - default/top" story.
| // ********* DOCS ONLY ********* // | ||
| /** | ||
| * A tag group on its own should always have a label. Labels can be placed either on top or on the side on the tags, but top labels are the default and are recommended because they work better with long copy, localization, and responsive layouts. |
There was a problem hiding this comment.
Just a typo:
"...on top, or on the side of the tags..."
| }, | ||
| ], | ||
| }; | ||
|
|
||
| /** | ||
| * A single quiet action button may be included at the end of a tag group if the action affects the entire group. Common actions include "show all", "show less", and "clear all". A counter of the number of tags can be included in the action button label if appropriate for the context. |
There was a problem hiding this comment.
Just a few typos: the commas & period for the "common actions" should be inside of the quotes.
"show all," "show less," and "clear all."
📚 Branch previewPR #3966 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-3966/index.html. |
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-tagsTemplate 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