feat(breadcrumb)!: spectrum 2 migration #3395
Conversation
Current changes on main, including bugfixes, to assist with testing with s2, until the base branch is updated with main.
The options and naming around densities has changed in s2. There is now: - Medium / default (was previously "compact") - Large (was previously default) - Multiline (existing) This updates the controls, classes, and tokens around that. It also sets the bulk of the new values for existing custom properties that have changed in s2.
Leverage changing custom property values for --large and --multiline, instead of creating new properties for every variant. This reduces the number of custom properties and slims down the CSS.
Pulls in most of the stories from main, and changes compact stories to large stories-- This commit can be modified or dropped once spectrum-two is up to date with all of the stories and template changes on main. This excludes BreadcrumbGroup, test.js, etc.
Finalize use of s2 tokens defined in design, including new tokens and changes. Adjusts which multiline tokens are applied.
🦋 Changeset detectedLatest commit: 74ba42e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
jawinn
added
S2
jawinn
changed the title
The new specs specified that title item can be customized in the multiline variant, with preferred sizing. This adds support for that in our template and adds a new Storybook control.
jawinn
force-pushed
the
jawinn/css-811-s2-breadcrumbs
branch
from
88c8ac3
to
a819f2b
Compare
File metricsSummaryTotal size: 4.28 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsbreadcrumb
colorarea
opacitycheckerboard
thumbnail
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3395--spectrum-css.netlify.app |
Adjusts some spacings and the tokens used to make sure that this is adhering to the tokens defined on the design spec. Of note, this helps fix: - Too much horizontal spacing beside truncated menu; there needed to be different spacing for text to separator and truncated menu to separator. To allow this be changed, some margins/paddings were moved around to different elements than before. - Uses correct start edge spacing (previously padding on the ul), which needs to be different for text items and the truncated menu item.
|
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: |
There was a problem hiding this comment.
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?
| --spectrum-breadcrumbs-font-weight: var(--spectrum-regular-font-weight); | ||
|
|
||
| --spectrum-breadcrumbs-font-size-current: var(--spectrum-font-size-100); | ||
| --spectrum-breadcrumbs-font-family-current: var(--spectrum-sans-font-family-stack); |
There was a problem hiding this comment.
Font-stack question: why does "Adobe Clean" render differently than adobe-clean? Using "Adobe Clean" looks more like the design, but our font stack renders adobe-clean because that's the first font in the stack.
Maybe not something that needs to be fixed, but wanted to point it out.
There was a problem hiding this comment.
Interesting. Looking at the rendered font under the computed tab, with the stack renders "Family name: Adobe Clean Black". This is the network resource, from Typekit. For "Adobe Clean" it'll use what is installed on ones machine, which renders the local file "Family name: Adobe Clean ExtraBold".
In the tokens for both S1 and S2, the 800 weight being used is extra bold, and 900 should be black:
--spectrum-extra-bold-font-weight: 800;
--spectrum-black-font-weight: 900;
It sounds like we need more info about the Typekit configuration and whether this is intentional or misconfigured. I'll create a Jira issue for that; CSS-1069.
| flex-flow: row nowrap; | ||
| align-items: center; | ||
| justify-content: flex-start; | ||
| flex: 1 0 0%; |
There was a problem hiding this comment.
This isn't something introduced in the migration, but I noticed this in the inspector and I'm wondering if there's any consequence to removing it:
| }, | ||
| ], | ||
| }; | ||
| DefaultNested.tags = ["!dev"]; |
There was a problem hiding this comment.
Noting that all the stories are showing in the sidebar since this branch doesn't know what "!dev" is, but I think that's fine.
There was a problem hiding this comment.
👍 Yep, that is intentional so this is ready for when spectrum-two is brought back up to date (which will be using the newer version of Storybook that supports these tags).
- update component height token used - remove two unused custom properties
| --spectrum-breadcrumbs-separator-spacing-inline: var(--spectrum-breadcrumbs-text-to-separator-multiline); | ||
| --spectrum-breadcrumbs-text-spacing-block-start: var(--spectrum-breadcrumbs-top-to-text-multiline); | ||
| --spectrum-breadcrumbs-text-spacing-block-end: var(--spectrum-breadcrumbs-top-text-to-bottom-text); | ||
| --spectrum-breadcrumbs-icon-spacing-block: var(--spectrum-breadcrumbs-top-to-separator-multiline) var(--spectrum-breadcrumbs-separator-to-bottom-text-multiline); |
There was a problem hiding this comment.
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.
| --spectrum-breadcrumbs-font-weight: var(--spectrum-regular-font-weight); | ||
|
|
||
| --spectrum-breadcrumbs-font-size-current: var(--spectrum-font-size-100); | ||
| --spectrum-breadcrumbs-font-family-current: var(--spectrum-sans-font-family-stack); |
There was a problem hiding this comment.
Interesting. Looking at the rendered font under the computed tab, with the stack renders "Family name: Adobe Clean Black". This is the network resource, from Typekit. For "Adobe Clean" it'll use what is installed on ones machine, which renders the local file "Family name: Adobe Clean ExtraBold".
In the tokens for both S1 and S2, the 800 weight being used is extra bold, and 900 should be black:
--spectrum-extra-bold-font-weight: 800;
--spectrum-black-font-weight: 900;
It sounds like we need more info about the Typekit configuration and whether this is intentional or misconfigured. I'll create a Jira issue for that; CSS-1069.
| }, | ||
| ], | ||
| }; | ||
| DefaultNested.tags = ["!dev"]; |
There was a problem hiding this comment.
👍 Yep, that is intentional so this is ready for when spectrum-two is brought back up to date (which will be using the newer version of Storybook that supports these tags).
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:
spectrum-Breadcrumbs--compactclass and associated custom properties are removed. The default (medium) breadcrumbs are now sized similar to what was previously called compact.spectrum-Breadcrumbs--largeclass. 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 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 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-multilineThe following mod custom properties have been renamed:
--spectrum-breadcrumbs-action-button-spacing-inline-startis now--spectrum-breadcrumbs-inline-start-to-truncated-menuto 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
titleHeadingSizeworks as expected and documentation around this is clearRegression testing
Validate:
To-do list