USWDS - Nav: Vertically center nav expander button #5267
Conversation
Styles were largely duplicated across
| @@ -209,20 +209,26 @@ $expand-less-icon: map-merge( | |||
| } | |||
| } | |||
|
|
|||
| &[aria-expanded="false"] { | |||
| &[aria-expanded] { | |||
There was a problem hiding this comment.
The diff here is a bit more complex than originally planned since I initially didn't notice that the positioning styles were largely duplicated across &[aria-expanded="false"] and &[aria-expanded="true"] selectors. I could have duplicated the top and transform styles for the second style selector, but I figured it'd be more maintainable (avoid similar such confusion in the future) to consolidate.
There was a problem hiding this comment.
Hey there @aduth
Thank you for tackling this! Overall I think it’s great. It fixes the bug in most views and I like the refactoring of the styles become more DRY.
Just two notes for you:
Padding issue
In desktop view, when the span reaches certain lengths, it seems like it’s long enough to cause a line break but not break into the line itself. Instead, it flows into the right padding and affects other header buttons as well. I believe this has to do with the display settings but setting it to block/inline-block causes the buttons to become different sizes.
This behavior exists on develop as well but, ideally, could be accomplished in this PR. Would you mind taking a crack at it?
Merge conflict
Looks like theres a merge conflict with the nav scss file. Would you mind pulling the latest and resolving that for us? Thanks!
There was a problem hiding this comment.
@aduth Thank you for this PR. This is looking good! There are a couple of merge conflicts so I am going to hold off on approval until those are resolved and I can check again.
I tested the following in Safari, Chrome, and Firefox:
-
Confirm icon is vertically centered in mobile with two lines of text
-
Confirm icon is vertically centered in desktop with two lines of text
Note
This is likely out of scope, but the alignment is off when the nav content is wider than the usa-nav element in the Default variant. However the alignment is a pre-existing problem in develop. If the fix is out of scope we should open a new issue to fix this item.
- Confirm icon is vertically centered in mobile with one line of text
- Confirm icon is vertically centered in desktop with one line of text
- Confirm icon is correctly aligned when item is expanded
- Confirm no regressions in hover state colors in both desktop and mobile
- Confirm no regressions in high contrast mode in both desktop and mobile
- Confirmed for all variants of the header
…ical-center-nav-expander
|
Resolved merge conflicts and will merge into feature branch once tests pass. Will continue testing there 👍 |
|
Thanks @mahoneycm and @amyleadem ! I thought I'd left a comment on this pull request earlier, but I must have been mistaken. Apologies for not following-up sooner. I had spent a little time trying to resolve the padding issue on desktop, but it turned out to be a little trickier than I anticipated. Since I've been rather slow to return to some of my pull requests, feel free to do whatever works best if you'd like to continue it along from your end (merge to another branch, push commits directly, close, etc). |
|
@aduth no worries at all! Thanks so much for this contribution. Planning on releasing this change with the upcoming |
Summary
Improve display of header navigation dropdown button for long links. The expander icon will now be vertically centered.
Preview link
Preview locally at http://localhost:6006/?path=/story/components-header--default
Problem statement
When navigation links include text which breaks to multiple lines (particularly in mobile navigation), the expander icon is shown at the bottom right of the link, which makes it more difficult to identify.
Solution
Vertically center the icon.
Testing and review
Verify icon placement in both mobile and desktop (since the icon is also used for expander menus on larger viewport navigation).