Skip to content

USWDS - Breadcrumb: Allow text to wrap on wrap variants#5497

Merged
thisisdano merged 3 commits into
developfrom
al-breadcrumb-wrap
Sep 28, 2023
Merged

USWDS - Breadcrumb: Allow text to wrap on wrap variants#5497
thisisdano merged 3 commits into
developfrom
al-breadcrumb-wrap

Conversation

@amyleadem

@amyleadem amyleadem commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

Summary

Fixed a bug that prevented text from wrapping to a new line in the .usa-breadcrumb--wrap variant.

Breaking change

This is not a breaking change.

Related issue

Closes #5257

Related pull requests

Changelog PR

Preview link

Problem statement

In the breadcrumb component, all list items (regardless of variant) receive a white-space: nowrap rule. This enables the expected behavior in the standard variant, but prevents text from wrapping In the .usa-breadcrumb--wrap variant.

Solution

By removing the blanket white-space: nowrap rule from list items and instead relying only on the "no-wrap" rule applied to .usa-breadcrumb__list in the standard variant, we can re-enable text wrapping in the .usa-breadcrumb--wrap variant.

image

Testing and review

In a variety of browsers:

  • Confirm that the breadcrumb text wraps as expected:
    1. Open the breadcrumb wrap variant
    2. Resize the window to ~550px wide
    3. Confirm that text wraps to a new line in the wrap variant and does not extend beyond the container.
  • Confirm that there are no regressions in the standard variant.

.usa-breadcrumb__list-item {
@include sr-only;
@include u-display("inline");
@include u-white-space("no-wrap");

@amyleadem amyleadem Sep 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be unnecessary for the standard variant because no-wrap is already declared on the .usa-breadcrumb__list element in line 71. The line 71 rule is applied only to :not(.usa-breadcrumb--wrap), so no override is necessary.

</li>
<li class="usa-breadcrumb__list-item usa-current" aria-current="page">
<span>Women-owned small business federal contracting program</span>
<span>Economically disadvantaged women-owned small business federal contracting program</span>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the text longer to better demonstrate the text wrap

@amyleadem
amyleadem marked this pull request as ready for review September 12, 2023 17:35

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Tested both variants in:

  • Safari 16.6
  • Firefox 118
  • Chromium 116

@mejiaj
mejiaj requested a review from thisisdano September 12, 2023 18:06

@mahoneycm mahoneycm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

  • Wrap variant properly wraps across different browsers
  • No visual regression on standard variant across browsers

Chrome visual quirk

On chrome, I noticed a visual quirk in the standard variant where the ellipses overlaps the breadcrumb chevron. The same behavior exists on develop so I don't think it should be a blocker for this PR. I was unable to reproduce on Firefox or Safari.

Screenshot 2023-09-14 at 10 01 13 AM

Let me know if you'd like me to create a new issue to track this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

USWDS - Breadcrumb: Override breadcrumb no-wrap on variant

4 participants