adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.88k stars 1.12k forks source link

Breadcrumb first crumb should not have padding #2212

Closed sirugh closed 2 months ago

sirugh commented 3 years ago

πŸ› Bug Report

We implemented Breadcrumbs across several "pages" within our app and noticed that there is padding applied to a crumb text to provide spacing between the text and the delimiter >. It seems to be a bug or potential oversight that there is an opinionated padding on the external sides of the element.

πŸ€” Expected Behavior

In my opinion, this padding does not belong on the first child/crumb of the Breadcrumbs component.

image

😯 Current Behavior

image

πŸ’ Possible Solution

For the first "crumb", set padding-left: 0;.

πŸ”¦ Context

We have several routes that use the breadcrumb component. Our designer has pointed out the extra padding, causing the breadcrumbs to not be left aligned with the other components on the page. While we could override it in the few places we implement breadcrumbs, it seems less scaleable a solution than fixing it at the source. If we did override it, we may also forget to do so when adding a new Breadcrumb, which will be extra work to remedy.

πŸ’» Code Sample

https://codesandbox.io/s/stupefied-https-wuc8d?file=/src/App.js

🌍 Your Environment

Software Version(s)
react-spectrum 3.12.0
Browser Chrome Version 92.0.4515.131 (Official Build) (x86_64)
Operating System Mac OSX 11.5.1

🧒 Your Company/Team

Adobe Commerce

πŸ•· Tracking Issue (optional)

snowystinger commented 3 years ago

Seems reasonable, thanks for bringing it up. I'll ask our Design team.

sirugh commented 3 years ago

Thanks @snowystinger. If they would like to have input from our designer, her name is Tina Moore - can find her on Slack or outlook, I'm sure.

I've also opened a PR for you, as linked above :)

snowystinger commented 3 years ago

Hey, so the designer who created this is unfortunately out this week. I should have an answer to you next week.

snowystinger commented 3 years ago

Hey! Talked with our design team today. They agree this should be alignable in the same way that Tabs are handled. We'll need to remove that first element left padding, and we'll likely need to apply a negative margin to the case where the first element is an icon so that it can maintain its hit area.

sirugh commented 3 years ago

@snowystinger cool, thanks for the follow up! I started a PR here that removes first element left padding, but I'm not certain about how to accomplish the second case.

Would it be a variable margin based on the size of that icon? Or are those icon/elements static? With a little guidance I can update my PR accordingly :)

snowystinger commented 3 years ago

The case with the icon needs to keep the padding but make it look like it's been removed by adding a negative margin of the same amount as the padding calc(-1 * var)

One thing we'll need to consider is that people have already worked around this issue instead of raising it, so this may be considered a breaking change. We'll need to discuss how to handle it. If you have any suggestions from working with it, feel free to add your thoughts.

sirugh commented 3 years ago

I'm actually not sure we want to remove margin on the icon as it gets a sort of shadow/border when hovered that would then extend further left than the left alignment. This screen is of the focus state but the shadow is the same area.

image

As far as I could tell, when I manually removed the padding, the clickable area remained the same. πŸ€”

snowystinger commented 3 years ago

That's how Tabs work, it extends outside the component. The icon and second level of text in breadcrumbs should align, the focus ring does not need to

Screen Shot 2021-08-20 at 3 59 25 PM Screen Shot 2021-08-20 at 3 59 16 PM
carolframpton commented 2 years ago

Our team would also like to see the issue, as first reported, fixed. This does not look good.

image

snowystinger commented 2 months ago

I'm going to close this as not doing in v3, design hasn't gotten to it and it'd be a breaking change or a very weird API. At this point people are probably using UNSAFE style to work around it.

Our work in s2 will make this much easier, you'll be able to specify the marginStart on the supported styles prop.