Skip to content

Conversation

cathybaptista
Copy link
Contributor

@cathybaptista cathybaptista commented Aug 29, 2024

Summary

Changelog for uswds/uswds#6037

Related PR

uswds/uswds#6037

Preview link

Header changelog

@cathybaptista cathybaptista changed the title Cb changelog 6037 USWDS-Site: Add changelog for Search input appears last in mobile header [#6037] Aug 29, 2024
@cathybaptista cathybaptista changed the title USWDS-Site: Add changelog for Search input appears last in mobile header [#6037] USWDS-Site - Add changelog for Search input appears last in mobile header [#6037] Aug 29, 2024
@cathybaptista cathybaptista changed the title USWDS-Site - Add changelog for Search input appears last in mobile header [#6037] USWDS-Site: Add changelog for Search input appears last in mobile header [#6037] Aug 29, 2024
@cathybaptista cathybaptista marked this pull request as ready for review August 29, 2024 18:31
@mahoneycm mahoneycm changed the base branch from main to release-3.9.0 September 3, 2024 21:04
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.

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.
Copy link
Contributor

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.

Copy link
Contributor

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

@cathybaptista
Copy link
Contributor Author

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.

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! Can you pull the latest from release-3.9.0 to clean up the files changed tab? I'll approve after that 😎

@cathybaptista
Copy link
Contributor Author

Hi, @mahoneycm! I updated the branch. Let me know if anything doesn't look right. :)

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

LGTM!

@amyleadem amyleadem requested review from thisisdano and removed request for mejiaj and CTGM-Bixal September 30, 2024 23:13
Copy link
Contributor

@mejiaj mejiaj left a 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.
Copy link
Contributor

@amyleadem amyleadem Oct 3, 2024

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.

Suggested change
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.

Copy link
Contributor

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?

Copy link
Contributor

@amyleadem amyleadem Oct 3, 2024

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"?

Copy link
Contributor

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.

cathybaptista and others added 2 commits October 3, 2024 12:28
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
@amyleadem amyleadem merged commit a11b0c1 into release-3.9.0 Oct 4, 2024
2 of 5 checks passed
@cathybaptista cathybaptista deleted the cb-changelog-6037 branch October 8, 2024 13:47
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.

5 participants