feat(breadcrumb)!: spectrum 2 migration

[jawinn]

Description

Breadcrumbs Spectrum 2 Migration

The breadcrumbs component is now updated to use the S2 specs and tokens. This includes updated spacing, heights, colors, font sizes, etc. This does not currently include the updated S2 icons.

There has been a major design change to the variant classes related to density and sizing, to align their terminology better with t-shirt sizes:

• The "compact" variant no longer exists. The spectrum-Breadcrumbs--compact class and associated custom properties are removed. The default (medium) breadcrumbs are now sized similar to what was previously called compact.
• There is a new "large" variant, which uses the spectrum-Breadcrumbs--sizeL class. This is sized similarly to what was previously the default.

The breadcrumb title can now be customized in the multiline variant using an additional element that uses the typography component's heading classes. This and its preferred sizing have been added to the documentation, a new Docs example, and a new Storybook control titleHeadingSize. Design notes for reference:

The title item can be customized in the multiline variant, but the preferred sizes are Heading (S), Heading (M), Heading (L) (default), and Heading (XL)

The component has been refactored to slim down and simplify its CSS and custom properties, by changing the values of existing custom properties for large and multiline variants.

The drag and drop class has been better documented, and its class now has a background color using drop zone tokens for now per design feedback: "Those two use cases are essentially one in the same, as the breadcrumb item becomes a "drop zone" for that interaction."

The following mod custom properties have been removed:

• --mod-breadcrumbs-action-button-spacing-block-between-multiline
• --mod-breadcrumbs-action-button-spacing-block-compact
• --mod-breadcrumbs-action-button-spacing-block-multiline
• --mod-breadcrumbs-block-size-compact
• --mod-breadcrumbs-block-size-multiline
• --mod-breadcrumbs-font-family-compact
• --mod-breadcrumbs-font-family-compact-current
• --mod-breadcrumbs-font-family-multiline
• --mod-breadcrumbs-font-family-multiline-current
• --mod-breadcrumbs-font-size-compact
• --mod-breadcrumbs-font-size-compact-current
• --mod-breadcrumbs-font-size-multiline
• --mod-breadcrumbs-font-size-multiline-current
• --mod-breadcrumbs-font-weight-compact
• --mod-breadcrumbs-font-weight-compact-current
• --mod-breadcrumbs-font-weight-multiline
• --mod-breadcrumbs-font-weight-multiline-current
• --mod-breadcrumbs-icon-spacing-block-between-multiline
• --mod-breadcrumbs-icon-spacing-block-compact
• --mod-breadcrumbs-icon-spacing-block-start-multiline
• --mod-breadcrumbs-text-spacing-block-between-multiline
• --mod-breadcrumbs-text-spacing-block-end-compact
• --mod-breadcrumbs-text-spacing-block-end-multiline
• --mod-breadcrumbs-text-spacing-block-start-compact
• --mod-breadcrumbs-text-spacing-block-start-multiline

The following mod custom properties have been renamed:

• --spectrum-breadcrumbs-action-button-spacing-inline-start is now --spectrum-breadcrumbs-inline-start-to-truncated-menu to help clarify what it is used for. The general action button inline spacing is already handled by --mod-breadcrumbs-action-button-spacing-inline.

CSS-811

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

• Breadcrumbs compared against S2 spec
• New t-shirt size "large" exists and "compact" variant no longer exists
• Small , medium, and multiline tested
• New control titleHeadingSize works as expected and documentation around this is clear
• New controls work as expected for truncated menu, root context, nav root text, and items (items was previously hidden)
• Updated testing preview covers all variants, including added hover and focus-visible

Regression testing

Validate:

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

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

2. If components have been modified, VRTs have been run on this branch:

• VRTs have been run and looked at.
• Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

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 other components, I have tested to make sure they don't break.
• If my change impacts documentation, I have updated the documentation accordingly.
• ✨ This pull request is ready to merge. ✨

[changeset-bot]

🦋 Changeset detected

Latest commit: 50fbb69

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/breadcrumb	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]

File metrics

Summary

Total size: 2.22 MB^*
Total change (Δ): 🟢 ⬇ 5.23 KB (-0.23%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Δ
breadcrumb	13.79 KB	🟢 ⬇ 2.86 KB

Details

breadcrumb

Filename	Head	Compared to base
index.css	13.79 KB	🟢 ⬇ 2.86 KB (-17.17%)
metadata.json	7.50 KB	🟢 ⬇ 2.37 KB (-24.05%)

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

[github-actions]

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

[jawinn]

I am seeing one issue where the text is rendering a little too high and those elements are greater than 32px tall (33.19px). This is related to line-height and the top and bottom padding token values, which is very similar to the discussion raised a few months back about button and line-height tokens. I'll bring this up with design again in Slack:

[rise-erpelding]

This looks really good! I only have a few small suggestions within the comments.

Also wondering: are the medium and large for breadcrumbs intended to be variants or t-shirt sizes? Is there some specific guidance somewhere that states that they can't or shouldn't be t-shirt sizes? I think the token specs make them seem more like variants, but the S2/Desktop spec makes them feel a bit more t-shirt size-like. It seems like, especially since --medium and --large are also used for specifying device size, it would be easier and less confusing to name them with the same conventions for t-shirt sizing, and that a future version of the component might follow t-shirt sizing even if this one doesn't. Thoughts?

[jawinn]

Do you think it's worth splitting --spectrum-breadcrumbs-icon-spacing-block into --spectrum-breadcrumbs-icon-spacing-block-start and --spectrum-breadcrumbs-icon-spacing-block-end versus setting them both in one like this? Same question for --spectrum-breadcrumbs-action-button-spacing-block.

[jawinn]

Also wondering: are the medium and large for breadcrumbs intended to be variants or t-shirt sizes? Is there some specific guidance somewhere that states that they can't or shouldn't be t-shirt sizes? I think the token specs make them seem more like variants, but the S2/Desktop spec makes them feel a bit more t-shirt size-like. It seems like, especially since --medium and --large are also used for specifying device size, it would be easier and less confusing to name them with the same conventions for t-shirt sizing, and that a future version of the component might follow t-shirt sizing even if this one doesn't. Thoughts?

I've gone ahead and moved things around so large is now --sizeL, and there are separate size and variant controls. I agree that this seems better given that these are trying to align more with t-shirt sizes (there is a whole specs section under "reason for token updates" that goes over it). The way it previously was a variant and is presented in the designs where it's either default(medium), large, or multiline had made it feel like a variant. But I think this was is more consistent and future-proof if additional sizes get added.

[jawinn]

There are two things that I'd like to get answers on before re-review:

• The action button is no longer displaying correctly on the spectrum-two branch. After this branch was rebased, the border on breadcrumb's quiet action button started showing. We'll have to look into what happened there and get that fixed on spectrum-two.
• There was no design for "dragged". I updated the docs around this option after I figured out what it's doing. I'll ask design for specs on this; the older guidelines were showing a background color change that I don't see.

[marissahuysentruyt]

This first pass was just TODOs now that spectrum-two got its mega-update (I know we're waiting on some fixes and questions before another deep review). Looks like we can probably refactor in a couple places to use Container and size.

[marissahuysentruyt]

I'm going to do the same as Rise, and leave a chunk of comments here (hopefully I'm not repeating her too much):

In package.json- I think there was a card to update the package name from "breadcrumb" (singular) to "breadcrumbs" (plural) (css-1080). Is that something we should/could do in this PR, given everything will be a breaking change, or should that be separate? This is just me being curious and doesn't block this PR or anything.

In breadcrumb.stories.js:

• Could we add the file extension back in the Template import? I thought I read in a recent PR that we we had a command (maybe in the chromatic add-on update PR?) that needed to be stricter about file extensions (although I'm not sure if that applies in story files 🤷‍♀️ ).
• Do you think we should use that isDragged type again? Could we just extend it?

isDragged: {
			...isDragged,
			name: "Show dragged item",
			description: "Breadcrumbs can have optional behavior to allow for drag and drop functionality. Setting this to true will style the second breadcrumb item as if something is currently being dragged on top of it.",
		},

• I haven't seen many cases where we have "undefined" in the default args (like variant: undefined). I like how you have the variant control with undefined being mapped in the actual dropdown to "Default," but I was expecting to just see "Large" selected in the title size control. Does writing the control this way (with an "undefined" option for title size) allow SWC to not even have the titleHeadingSize prop/attribute or something?
  • Nothing about what you have is wrong! It was just something I noticed. I guess I imagined we'd just treat the title heading sizes like the size arg, where we just define what the default title heading size is in those default args i.e. titleSizeHeading: "l".
• Just want to say that I appreciate all of the additional documentation you added!

We'll need to probably update the breadcrumbs.test.js file to remove the compact variant (which I'm sure you already know- just noting it here in case).

[jawinn]

I've updated this to address all of the review comments (see added commits). I refactored the breadcrumb items and controls so all examples shown on the Docs can now be created by changing options on the Default story. There are now truncated menu and root context controls.

I also reworked the tests per the comments, so please re-check the testing preview.

A few responses to some of the review comments:

---

It would be nice to add a test case for sizing with the nested variant, to be able to confirm that the action button will change its size when the breadcrumb does.

Large is now its own section in the tests / testing preview, so this is now covered.

...I was expecting to just see "Large" selected in the title size control. Does writing the control this way (with an "undefined" option for title size) allow SWC to not even have the titleHeadingSize prop/attribute or something?

This allows testing what it looks like with text directly in the element, without using an additional nested element with the typography class. The text gets set to large by default in the CSS using a token. This keeps this kinda backwards compatible / as a fallback, keeping the typography dependency as optional.

Do you think we should use that isDragged type again? Could we just extend it?

Good idea. Done.

[rise-erpelding]

This looks great, thanks for all the hard work on the Storybook controls and the tests!

One really small thing I noticed:

• Should the large nested variant display the medium size action button? I know the Docs say that the "When using the large size, the truncated menu action button should also use the large size." I see the large variant using the M action button here on the Docs page, in tests, and for the story in Storybook default mode.
  

A few other smaller, non-blocking things from the tests:

• We have default and large containers in the tests, so do we still need the "Sizing" container at the bottom?
• Multiline comes in 4 sizes, but it looks like we're only testing small and default/medium. Would we want to test large and extra-large as well?

[marissahuysentruyt]

This looks great! The documentation and controls you added help clarify breadcrumbs so much. Thank you! I have a few questions that maybe aren't necessarily blockers, but I wanted to bring to your attention. For that question I had about the title size/default-no-clasess-multiline- you may already have gotten clarification from design on that one, so I apologize if that's a repeat question!

Looking at the Figma desktop file for breadcrumbs...there's no dragged variant for the items. Is that something we need to keep/is dragged still supported in S2? It very well might be, just wanted to ask.

Do you think we need a focus state story on the docs page? Some of the other stories have them on their docs pages, but you did add the focus-visible test, so it's not like we haven't thought about it.

[jawinn]

@rise-erpelding

Should the large nested variant display the medium size action button?

The args for action button sizing are now updated to work with the change in variants, so s = multiline, m = size medium, l = size large.

We have default and large containers in the tests, so do we still need the "Sizing" container at the bottom?

Yes, good idea. I removed that by adding withSizes: false to the test.

Multiline comes in 4 sizes, but it looks like we're only testing small and default/medium. Would we want to test large and extra-large as well?

This now has its own test. Comments here on Marissa'a similar suggestion.

---

@marissahuysentruyt

Looking at the Figma desktop file for breadcrumbs...there's no dragged variant for the items. Is that something we need to keep/is dragged still supported in S2? It very well might be, just wanted to ask.

That was already something I confirmed with design about and they provided guidance on the dragged background color (some notes in PR description). I pinged them again to ask about adding this to the spec, as this will help with reviewing and comparing.

Do you think we need a focus state story on the docs page? Some of the other stories have them on their docs pages, but you did add the focus-visible test, so it's not like we haven't thought about it.

I don't think we need to show this on the Docs page in most cases, since we're using a storybook-only test class for VRT testing purposes for some of these interactions like focus, hover, active, etc. That could be a little confusing for referencing the markup to see a class not included in the CSS and it is not totally accurate in all cases. I don't see most button-like and form components Docs showing this currently. I think Textfield is one of the exceptions, since the docs were necessary to avoid some historical confusion and also show the quiet/non-quiet difference.

[rise-erpelding]

This update addresses all the things I'd noticed before!

I did notice one very minor thing by accident, because I had previously been checking cjk on another PR and had the locale flipped to Japan already:

When I switch to a cjk language, the multiline test/story with all the different-sized headings, it looks like the font size for .spectrum-Heading--sizeX shrinks a bit. There's no font-size adjustment for .spectrum-Breadcrumbs-item for cjk though so this font size stays the same. That makes it so that if no heading size is specified, the default heading size no longer matches any of the size options, if that makes sense. Basically, every single one of the current items in this screen shot has a different font for cjk (which you can kind of see visually if you look closely), instead of the default heading size always keeping pace with the large. The default with no heading size specified is 28px while the large is 25px. (It's also rendering different font stacks.)

I'm unsure if this is something that we can/should fix or whether maybe cjk was left out of the design for this? So I'll leave it to your discretion if it's something that needs to be addressed here and won't let it block my approval!

[jawinn]

Re: the CJK comments, Breadcrumbs does not have CJK styles defined in the spec, while the Heading component does. I've created a followup issue CSS-1113 to track, and started a conversation with the design team.

Also, about the "breadcrumb" vs "breadcrumbs" component naming, I found the existing issue CSS-1080; that would definitely be separate work as that will affect a lot of things.

[marissahuysentruyt]

This is looking good to me- thanks for addressing all of my comments!

Just a quick (non-blocking) question- how did you know what version to bump the breadcrumbs tokens? or did that update happen in the rebase (that's sort of what I'm thinking)?