Skip to content

USWDS - Footer: Show overflow (for focus rings) #6093

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

USWDS - Footer: Show overflow (for focus rings) #6093

wants to merge 4 commits into from

Conversation

6TELOIV
Copy link
Contributor

@6TELOIV 6TELOIV commented Oct 4, 2024

Summary

Remove overflow: hidden from usa-footer to let focus ring show.

Breaking change

None

Related issue

Closes #6092

Related pull requests

None

Preview link

None

Problem statement

Focus rings are visually cut off at the top in the footer.

Solution

Removing overflow: hidden allows focus rings to show at the edge of the footer.

Testing and review

Should visually review the various recommended usa-footer layouts to make sure this doesn't introduce any overflow bugs.

@6TELOIV
Show focus ring on footer.
b6b835e 
This `overflow: hidden` doesn't seem to serve any purpose, and cuts off the focus ring of elements at the edge of the footer.
amyleadem
amyleadem suggested changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@amyleadem amyleadem left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for submitting this PR @6TELOIV. The focus outline now appears on all sides of the link elements.

I noticed that there is a small visual regression in the mobile view, where the list element borders are now visible outside the footer container. I've attached a screenshot that demonstrates this:

image

This is likely not typically visible in project environments where the footer spans from edge to edge, but it is something that should be resolved. I do not see any other visual regressions after removing overflow: hidden.

Please let us know if you have any questions.

@6TELOIV
Copy link
Contributor Author

6TELOIV commented Nov 13, 2024

I think the solution to that regression is to move border-top: 1px solid #adadad; to the .usa-footer__primary-link and .usa-footer__primary-link--button classes. Currently they're on the grid item, but should probably be on the child instead (based on the design).

Additionally, this fixes a different visual inconsistency in the Slim footer. notice the difference in length of the border on the primary links vs the contact links.

Version Image
Current image
Fixed image

@6TELOIV 6TELOIV requested a review from a team as a code owner November 13, 2024 17:13
@6TELOIV 6TELOIV requested a review from amyleadem November 13, 2024 17:16
mahoneycm
mahoneycm suggested changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Thanks for keeping up with this @6TELOIV ! The default and slim variants are looking great!

Left one suggestion to resolve a big footer variant regression.

@6TELOIV 6TELOIV requested a review from mahoneycm December 2, 2024 21:29
cathybaptista
cathybaptista approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@cathybaptista cathybaptista left a comment

Choose a reason for hiding this comment

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

@6TELOIV and @mahoneycm , I'm not seeing any regressions with this approach. Thanks for all of your work! :)

mahoneycm
mahoneycm approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution and updates @6TELOIV !

  • Footer no longer hides focus ring using example markup provided
  • No perceived visual regression across variants and breakpoints
  • Code changes are minimal and logical

This was referenced Dec 3, 2024
Merged
Merged
@amyleadem
Copy link
Contributor

Thanks for the update @6TELOIV. I found one more minor visual regression in the slim footer (see screenshot below).

@mahoneycm, @cathybaptista, @mejiaj
I opened up PR #6237 to explore a solution that removes the mobile x-padding from .usa-footer__primary-container. This could be considered scope creep, so I partitioned it out for now. However it solves two problems at once:

  1. Fixes the (minor) visual regression from this PR
  2. Fixes the pre-existing issue in the slim footer variant that caused the top border of the list items to not extend the full width of the component.

If we like this solution, we can either incorporate that change in this branch or just merge in that PR. Let me know what you think.

Regression in slim footer variant:
image

@amyleadem
Copy link
Contributor

@6TELOIV, if you get a chance, can you review the change in #6237?

@6TELOIV
Copy link
Contributor Author

6TELOIV commented Dec 5, 2024

@amyleadem sorry, I haven't done reviews of my own before, usually I'm requesting others to review! I think #6237 makes sense and just merging that and closing this one seems good (correct me if I'm misunderstanding anything here!).

@amyleadem
Copy link
Contributor

Thanks for your work and diligence on this @6TELOIV! Going to close this PR in favor of #6237, which has an additional fix for the list item padding in the slim footer.

@amyleadem amyleadem closed this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CTGM-Bixal CTGM-Bixal Awaiting requested review from CTGM-Bixal

@mejiaj mejiaj Awaiting requested review from mejiaj

@amyleadem amyleadem Awaiting requested review from amyleadem

2 more reviewers

@mahoneycm mahoneycm mahoneycm approved these changes

@cathybaptista cathybaptista cathybaptista approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

USWDS - Bug: Focus ring in footer links is cut off
5 participants
@6TELOIV @amyleadem @mahoneycm @cathybaptista @mejiaj