USWDS-Site - Banner: Remove duplicated styles so banner highlight shows completely on tab. #2743

cathybaptista · 2024-07-05T16:18:27Z

Summary

Removed duplicate styles for .usa-banner__button from uswds-site that were also in uswds. Now, the banner appears with the highlight on all four sides when the user tabs to the banner.

Related issue

Closes #1962

Preview link

home page

Problem statement

This issue relates to being able to test issue #5013 (https://github.com/uswds/uswds/pull/5013)](jhancock532:banner-focus-outline-styles).

#1962 was opened to resolve duplicated styles in uswds-site vs uswds that were causing the banner focus to only appear on 1 side for mobile devices. Ideally, the banner focus appears on all sides.

Rendering only an underline border on focus causes confusion to users with vision difficulties who have scaled the page to 400% zoom.

Solution

Before:

After:

Remove duplicate styles in uswds-site that also appear in uswds so that the correct style is shown and not overridden.

This approach was chosen because there is no need to have those duplicate styles in uswds-site when they are already in uswds, especially if they cause issues.

Testing and review

Open preview link and compare it to https://designsystem.digital.gov/. The border should appear on all four sides.
Confirm that style changes for .usa-banner__button make sense confirm that nothing got removed from uswds-site that also does not exist in uswds.
Confirm visual regression.

To reproduce:

pull the branch cb-remove-duplicate-uswds-styles
this issue appears on the main page
select the top banner (An official website of the United States government) and ensure that the blue outline appear on all four sides.
I am just looking for confirmation that it works.

mahoneycm

@cathybaptista Great start here Cathy! I left a couple change requests based on some regressions I noticed as well as additional styles we can remove.

I also have a question about the scope of the issue. Does this issue aim to address all duplicate styles in _uswds-theme-custom-styles.scss? Or is it scoped specifically to banner styles? If it's scoped to banner styles, should we address the additional duplications or are we primarily focusing on banner button?

In either case it look like there are additional styles we can potentially remove.

CC: @mejiaj since he is the issue submitter

css/_uswds-theme-custom-styles.scss

mahoneycm · 2024-07-10T15:11:44Z

From the issue:

@mahoneycm this is strictly scoped to duplicate banner styles.¹

Sounds like we look at the additional banner classes with potential duplicate styles, such as usa-banner__header and others.

https://github.com/uswds/uswds-site/issues/1962#issuecomment-2220740435 ↩

cathybaptista · 2024-07-17T21:25:07Z

@mahoneycm , I was able to remove many styles. Here is the list. I have checked that these removals don't cause any problems that I can see, but I would love a more experienced eye on this just to be 100% sure I didn't remove a style that is causing issues.

SOOO many thanks!

.usa-banner__icon
.usa-banner__button-text
@include at-media-max("tablet") {
.usa-banner__inner
.usa-banner__header--expanded
.usa-banner__header-flag
@include set-link-from-bg(
.usa-banner__header
.usa-banner__inner
.usa-banner__lock-image
.usa-banner__guidance
$banner-icon-close:
@include at-media("tablet") {

mahoneycm

Hey there @cathybaptista! Getting closer. It looks like there are additional duplicate styles that we can remove. I tagged some that stood out to me.

Would you mind ensuring we can safely remove these and giving this another pass to see if there are additional styles that are safe to remove?

mahoneycm · 2024-07-23T16:04:39Z

css/_uswds-theme-custom-styles.scss

-      min-height: 0;
-    }
-  }
-
  .usa-banner__header-text {
    @include u-margin-y(0);


Suggestion: Removing this margin does cause any regression of styles. Margin is set to 0 in banner styles lines 157-158.

Suggested change

@include u-margin-y(0);

mahoneycm · 2024-07-23T16:14:39Z

css/_uswds-theme-custom-styles.scss

@@ -2906,11 +2840,6 @@ $theme-table-column-gap: 4;
      middle,
      $site-banner-background-color
    );


Note: These styles are required to maintain proper color contrast of the chevron in mobile views

mahoneycm · 2024-07-23T16:29:10Z

css/_uswds-theme-custom-styles.scss

Chore: Run npm run prettier:scss. This will help to cleanup some white space and minor formatting issues 👍

mahoneycm · 2024-07-23T16:45:04Z

css/_uswds-theme-custom-styles.scss

-    }
-
-    &[aria-expanded="false"] {
-      background-image: none;
    }

    &[aria-expanded="true"] {
      background-image: none;


Suggestion: We can remove the background-image here. It's declared in banner styles lines 286-290

Suggested change

background-image: none;

mahoneycm · 2024-07-23T16:50:55Z

css/_uswds-theme-custom-styles.scss

-          @include u-pin("right");
-        }
-      }
-
      @include at-media("tablet") {


Suggestion: I'm unable to tag the correct lines because they are unedited but it looks like we can safely remove lines 2881-2887.

- height: auto; - padding: units(0); - position: relative; - &:after { - position: absolute; - }

These are set in banner styles lines 332-338

mahoneycm · 2024-07-23T17:14:10Z

css/_uswds-theme-custom-styles.scss

-      $site-banner-background-color,
-      $site-banner-link-color,
-      $context: $banner-context
-    );

    line-height: line-height($site-banner-font-family, 2);
    margin-bottom: units(0);


Suggestion: Remove lines 2845-2855. Unable mark as a code suggestion due to unchanged lines.

- margin-bottom: units(0); - margin-top: units(2px); - text-decoration: underline; - .usa-banner__header--expanded & { - display: none; - } - @include at-media("tablet") { - display: none; - }

Maintained by banner styles lines 172-182

cathybaptista and others added 2 commits July 3, 2024 17:52

remove duplicated styles so banner highlight shows.

c85c32a

Update package-lock

e085c99

cathybaptista marked this pull request as ready for review July 5, 2024 18:00

cathybaptista requested review from mejiaj, mahoneycm, amyleadem and RachelCorsino July 5, 2024 18:01

amyleadem changed the title ~~USWDS-Site-banner-remove duplicated styles so banner highlight shows completely on tab.~~ USWDS-Site - Banner: Remove duplicated styles so banner highlight shows completely on tab. Jul 5, 2024

mahoneycm requested changes Jul 10, 2024

View reviewed changes

css/_uswds-theme-custom-styles.scss Show resolved Hide resolved

css/_uswds-theme-custom-styles.scss Outdated Show resolved Hide resolved

css/_uswds-theme-custom-styles.scss Outdated Show resolved Hide resolved

mahoneycm mentioned this pull request Jul 10, 2024

USWDS-Site: Remove duplicate banner styles #1962

Open

cathybaptista force-pushed the cb-remove-duplicate-uswds-styles branch from 9c7c0bc to e085c99 Compare July 18, 2024 01:57

cathybaptista added 2 commits July 17, 2024 22:14

remove other styles.

96a41a0

add back accordion style.

70aa979

cathybaptista requested a review from mahoneycm July 18, 2024 14:39

cathybaptista added 2 commits July 18, 2024 11:52

put this style back.

f743cb6

remove the set-link-from-bg mixin.

ad0591f

mahoneycm requested changes Jul 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USWDS-Site - Banner: Remove duplicated styles so banner highlight shows completely on tab. #2743

USWDS-Site - Banner: Remove duplicated styles so banner highlight shows completely on tab. #2743

cathybaptista commented Jul 5, 2024 •

edited

Loading

mahoneycm left a comment •

edited

Loading

mahoneycm commented Jul 10, 2024

cathybaptista commented Jul 17, 2024 •

edited

Loading

mahoneycm left a comment

mahoneycm Jul 23, 2024

mahoneycm Jul 23, 2024

mahoneycm Jul 23, 2024

mahoneycm Jul 23, 2024

mahoneycm Jul 23, 2024

mahoneycm Jul 23, 2024

USWDS-Site - Banner: Remove duplicated styles so banner highlight shows completely on tab. #2743

Are you sure you want to change the base?

USWDS-Site - Banner: Remove duplicated styles so banner highlight shows completely on tab. #2743

Conversation

cathybaptista commented Jul 5, 2024 • edited Loading

Summary

Related issue

Preview link

Problem statement

Solution

Testing and review

mahoneycm left a comment • edited Loading

Choose a reason for hiding this comment

mahoneycm commented Jul 10, 2024

Footnotes

cathybaptista commented Jul 17, 2024 • edited Loading

mahoneycm left a comment

Choose a reason for hiding this comment

mahoneycm Jul 23, 2024

Choose a reason for hiding this comment

mahoneycm Jul 23, 2024

Choose a reason for hiding this comment

mahoneycm Jul 23, 2024

Choose a reason for hiding this comment

mahoneycm Jul 23, 2024

Choose a reason for hiding this comment

mahoneycm Jul 23, 2024

Choose a reason for hiding this comment

mahoneycm Jul 23, 2024

Choose a reason for hiding this comment

cathybaptista commented Jul 5, 2024 •

edited

Loading

mahoneycm left a comment •

edited

Loading

cathybaptista commented Jul 17, 2024 •

edited

Loading