-
Notifications
You must be signed in to change notification settings - Fork 194
USWDS-Site: Add changelog for Search input appears last in mobile header [#6037] #2798
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some potential changes here. It'd be a good idea to check in with the team in a sync or async in a slack thread before moving forward!
- date: NNNN-NN-NN | ||
summary: Reordered header items so that search bar appears last in mobile header. | ||
summaryAdditional: Now, the header is in compliance with WCAG criterion 2.4.3 (Focus order) in mobile view. | ||
Users that want to keep the search bar at the top of the menu will need to adjust their markup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: How should we recommend users restore the previous order?
If we suggest adjusting markup: It will also affect their desktop view.
If we suggest using CSS order: It will no longer be compliant with WCAG 2.4.3 (Focus order)
criterion.
It might be a good idea to add to a dev sync or office hours discussion to discuss with the team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
We should include whichever we decide in the 3.9.0
release notes
Change the summaryAdditional to If you would like to visually keep the search bar at the top of the menu, you will need to reorder your markup in the mobile view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Can you pull the latest from release-3.9.0
to clean up the files changed tab? I'll approve after that 😎
b2ff0fb
to
9d9e4ea
Compare
Hi, @mahoneycm! I updated the branch. Let me know if anything doesn't look right. :) |
…nto cb-changelog-6037
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content looks good. Approving since Charlie's out and could use an additional peer review.
@@ -2,6 +2,15 @@ title: Header | |||
type: component | |||
changelogURL: | |||
items: | |||
- date: NNNN-NN-NN | |||
summary: Removed the CSS order property from mobile header navigation. | |||
summaryAdditional: Now, the visual order of the component matches the tab order. If you would like to visually keep the search bar at the top of the menu, you will need to reorder your markup in the mobile view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Based on @thisisdano's comment, wanted to suggest a possible edit that explains that action is only necessary if your project needs to keep the search bar at the top of the nav.
summaryAdditional: Now, the visual order of the component matches the tab order. If you would like to visually keep the search bar at the top of the menu, you will need to reorder your markup in the mobile view. | |
summaryAdditional: Now, the visual order of the component matches the tab order. This is marked as a breaking change, but you only need to change your project code if you want to visually keep the search bar at the top of the menu. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want visually keep
here? Or should it be keep
instead for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point. "Visually keep" doesn't paint the full picture since it ignores that it will change the non-visual experience. It is a bit nuanced since non-visually nothing has changed, so "keep" is inaccurate in that sense ("move" might be a more accurate verb there), but I think that most users would find "keep" to be the clearest.
Want me to go ahead and make the update to "keep"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thisisdano Went ahead and updated this to "keep". Please let me know if you'd like any changes.
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
Summary
Changelog for uswds/uswds#6037
Related PR
uswds/uswds#6037
Preview link
Header changelog