-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(breadcrumb): center breadcrumb itemLink #2961
fix(breadcrumb): center breadcrumb itemLink #2961
Conversation
🦋 Changeset detectedLatest commit: 5db98f7 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 |
🚀 Deployed on https://pr-2961--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.63 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsbreadcrumb
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
d95a842
to
aba3099
Compare
aba3099
to
0e1bd8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@rise-erpelding- I struggled with this. I pushed up the
I'm not sure. In Chrome, if I hover over |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
08319c0
to
525f3fd
Compare
- although the .spectrum-Breadcrumb-item was aligned center, the items within it were not. This should align any children to the center of the parent item, and correct the visual misalignment between the text/button and the separator icon - chore(breadcrumb): create changeset
525f3fd
to
5db98f7
Compare
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 hasdisplay: 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 thespectrum-Breadcrumbs
parent element hasalign-items: center
, that was only affecting thespectrum-Breadcrumbs-item
element, and not thespectrum-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
tospectrum-Breadcrumbs-itemLink
resolves the height increase and corresponding text node misalignment. Additionally,align-self: center
was added to thespectrum-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
spectrum-Breadcrumbs-itemLink
elements.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 containeralign-self: center
turned off, measure the heights ofspectrum-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 talleralign-self: center
in the inspector, and remeasure all breadcrumb item links. All should be the same height nowalign-self: center
better aligns the text nodes to the separator icons, and keeps the height ofspectrum-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
breadcrumbs/breadcrumbs.stories.js
, add the following toDefault.args.items
(add this before the last item):align-self: center
property. You should see the same behavior as with the text nodes, wherealign-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:
Screenshots
To-do list