fix(breadcrumb): center breadcrumb itemLink

[marissahuysentruyt]

Description

During the SWC implementation of breadcrumbs, it was noticed that there might be some misalignment of the breadcrumb items, specifically for large/mobile scale:

Screen.Recording.2024-07-17.at.8.58.34.AM.mov

In the video above, when breadcrumbs is toggled to the mobile scale, the margin around the separator icon increases from 19px to 25px. That margin causes the spectrum-Breadcrumbs-item element to increase in height in comparison to the last breadcrumb item, which has display: none set on its corresponding separator icon. You can see in the screenshots below that the last breadcrumb item is shorter in height without the additional margin increase from the icon:

Because of the increase in height, as well as the margins placed on the spectrum-Breadcrumbs-itemLink element, you can see that element increases in height, filling any remaining space. Although the spectrum-Breadcrumbs parent element has align-items: center, that was only affecting the spectrum-Breadcrumbs-item element, and not the spectrum-Breadcrumbs-itemLink element that houses the text node. As the item link element grows, you can see the visual misalignment of text to the separator icon because the text node was not centered in the item link element. This also leads to the first few breadcrumbs' text to look misaligned to the last breadcrumb text:

Adding align-self: center to spectrum-Breadcrumbs-itemLink resolves the height increase and corresponding text node misalignment. Additionally, align-self: center was added to the spectrum-ActionButton in use cases where an icon is used.

before:

after:

The example below is from the Spectrum CSS Storybook, toggling between desktop and mobile scale:

Screen.Recording.2024-08-02.at.10.02.25.AM.mov

This example below uses the preview URL from the SWC breadcrumb PR, adding align-self: center in the inspector, and toggling it on and off:

Screen.Recording.2024-08-02.at.9.42.09.AM.mov

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

• Visit the breadcrumbs default story page
• Toggle the scale to "large" for mobile viewing, and turn on both the border and measurement tools

• Inspect one of the spectrum-Breadcrumbs-itemLink elements.
• Toggle the align-self: center in the inspector. You should see the borders of the item link elements change, and the text within them move up, away from the center line of the container

• With align-self: center turned off, measure the heights of spectrum-Breadcrumbs-itemLink. You should see a height difference with the first two items, in comparison to the last item/current breadcrumb. The first two items will be taller
• Turn on align-self: center in the inspector, and remeasure all breadcrumb item links. All should be the same height now
• In the Storybook controls, change the variant to compact or multiline. Repeat the process of checking that align-self: center better aligns the text nodes to the separator icons, and keeps the height of spectrum-Breadcrumbs-itemLink the same across all breadcrumb items (note that the final breadcrumb item in the multi-line variant should have a different height)

Additional validation

• Pull down the branch and run locally
• In breadcrumbs/breadcrumbs.stories.js, add the following to Default.args.items (add this before the last item):

  {
    iconName: "FolderOpen",
    isDisabled: true,
  },

• Visit the breadcrumbs default page (with the large scale, borders on, and measurement on)
• Back in the browser, you should now have additional breadcrumb with a disabled action button and folder icon
• Inspect the action button and toggle the align-self: center property. You should see the same behavior as with the text nodes, where align-self: center turned on ensures the action button is centered with the separator, and that property turned off leaves the action button unaligned.

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.

Screenshots

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: 5db98f7

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	Patch

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]

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

[github-actions]

File metrics

Summary

Total size: 4.63 MB^*
Total change (Δ): ⬆ 0.07 KB (0.00%)

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

Package	Size	Δ
breadcrumb	16.64 KB	⬆ 0.02 KB

Details

breadcrumb

File	Head	Base	Δ
index-base.css	16.64 KB	16.62 KB	⬆ 0.02 KB (0.13%)
index-vars.css	16.64 KB	16.62 KB	⬆ 0.02 KB (0.13%)
index.css	16.64 KB	16.62 KB	⬆ 0.02 KB (0.13%)
metadata.json	9.78 KB	9.78 KB	-

* 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.

[rise-erpelding]

Thank you for a great explanation of why this is happening and what it takes to fix it!

One really small thing I'm still questioning is whether we definitely need that align-self: center; on > .spectrum-ActionButton for the icon breadcrumb, the icon looks misaligned with or without it (pretty sure it's probably centered the way we'd want it, but I think it maybe looks a little wonky just because it's a different size than the text completely), and it's hard to gauge by measuring the size differences in the .spectrum-Breadcrumbs-item children as we did in the validation instructions for the text breadcrumbs... is there a better way to measure that this is centered that I maybe missed?

I'm also curious, if we do want to make sure that the action button is center-aligned (which most likely we do), can we take care of it by doing an align-items: center; on the parent (.spectrum-Breadcrumbs-item) rather than the individual children (.spectrum-Breadcrumbs-itemLink and .spectrum-Breadcrumbs-item > .spectrum-ActionButton), or would that cause some kind of unintended side effect?

[marissahuysentruyt]

can we take care of it by doing an align-items: center; on the parent (.spectrum-Breadcrumbs-item) rather than the individual children (.spectrum-Breadcrumbs-itemLink and .spectrum-Breadcrumbs-item > .spectrum-ActionButton), or would that cause some kind of unintended side effect?

@rise-erpelding- I struggled with this. I pushed up the align-items: center for spectrum-Breadcrumbs-item. Personally, I think that's the best way to do this in the long run. The reason I went with the align-self was because I was specifically trying to find a fix just for the mobile scale (which was where this was noticed originally). I guess I was concerned that align-items: center on Breadcrumb-item was moving the separators in desktop, centering them. I don't think that's a bad thing- it's still technically got all of their margin, just centered within spectrum-Breadcrumbs-item now. But if I look at the S1 design file, and even the S2 file, they're not technically centered? I thought I read somewhere that the designs (at least for S2 right now) were geared towards desktop, so I was trying not to touch desktop and just fix it for mobile (although the action button is now centered in desktop with that too. 🤦‍♀️ ). What do you think? I'm definitely open to the align-items: center fix, but maybe I got hung up on the "mobile" part.

is there a better way to measure that this is centered that I maybe missed?

I'm not sure. In Chrome, if I hover over align-items: center in the inspector, it will give me a grid line. Firefox does something sort of similar, but just extends the bounding boxes (it's definitely more "by eye" in Firefox).

[rise-erpelding]

Ok, now that I'm hovering in the right spot I can see it centering the items!

I was on the fence about align-self vs. align-items but after hearing your thoughts I agree with you and think align-items is a better way to go in this case. Nice work and thanks again for documenting all the things so clearly on this bug!