Skip to content

USWDS - Pages: Avoid reordering content in Documentation example #5681

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 - Pages: Avoid reordering content in Documentation example #5681

wants to merge 4 commits into from

Conversation

mejiaj
Copy link
Contributor

@mejiaj mejiaj commented Dec 15, 2023 •
edited
Loading

Summary

Documentation page structure follows logical order. Sidenav on page template now follows logical HTML order on mobile. Previously, the sidenav was shifted after the content, which did not follow the HTML order.

Breaking change

This is a potentially breaking change.
Removed template classes

  • usa-layout-docs__main: This is the class that had the styles that reordered element.
  • usa-layout-docs: This selector unused in current styles.

Caution

Users who rely on or extended the classes listed above would be negatively affected. See #5681 (comment).

Related issue

Closes #4594.

Related pull requests

Changelog entry: uswds/uswds-site#2495.

Preview link

Preview links; test all screen sizes:

Current (develop) Feature branch
Default Preview

Sidenav now comes before content on small screens. Previously, it was reordered after content.

Problem statement

In mobile, tab order doesn't match visual order in the documentation page example.

Solution

Removed styling that re-ordered the layout.

Testing and review

  • Sidenav should come before content in small screens (currently after)
  • Sidenav should be on left on larger screens.

@mejiaj mejiaj marked this pull request as ready for review December 15, 2023 21:23
@mahoneycm

This comment was marked as resolved.

Sign in to view
@mejiaj
Copy link
Contributor Author

mejiaj commented Dec 18, 2023

Hey @mejiaj, one thing that's not clear from the PR description that will resolve some of my questions is why we're making this change.

Is this to match default sidenav behavior when implemented on other pages?

I've updated the summary. Feel free to edit for clarity.

This change strictly affects the Documentation page example. We're making this change because in small screens the visual display does not match the tab order. Currently the sidenav is placed before the content, but then reordered to after on mobile.

mahoneycm

This comment was marked as resolved.

Sign in to view

@mejiaj mejiaj requested a review from mahoneycm December 18, 2023 18:36
amyleadem

This comment was marked as resolved.

Sign in to view

Remove unused class
91213f6 
Removed `usa-layout-docs` since its placement doesn't match BEM standards.
@mejiaj mejiaj requested a review from amyleadem January 2, 2024 21:58
@mejiaj mejiaj mentioned this pull request Jan 3, 2024
Open
3 tasks
@mejiaj mejiaj requested a review from mahoneycm January 3, 2024 21:32
@mejiaj
Copy link
Contributor Author

mejiaj commented Jan 3, 2024

@amyleadem @mahoneycm addressed comments, thank you. PR is ready for re-review.

amyleadem
amyleadem suggested changes Jan 4, 2024
View reviewed changes
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.

This is looking good. I added one small request to remove an undefined class. Other than that, I think it is good!

packages/templates/usa-documentation/includes/_usa-docs-inner.twig Outdated
@@ -49,7 +49,7 @@

</div>

<main class="usa-layout-docs__main desktop:grid-col-9 usa-prose usa-layout-docs" id="main-content">
<main class="usa-layout-docs__main desktop:grid-col-9 usa-prose" id="main-content">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also remove the usa-layout-docs__main class?

Suggested change
<main class="usa-layout-docs__main desktop:grid-col-9 usa-prose" id="main-content">
<main class="desktop:grid-col-9 usa-prose" id="main-content">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amyleadem I've removed the class in 66decc0. I've also bumped this up to potentially breaking change and noted in the PR description.

Mainly because:

  1. This is a markup change (even if minor)
  2. And because classes have been removed it could impact users that have relied on these classes for custom styling.

@amyleadem
Update packages/templates/usa-documentation/includes/_usa-docs-inner.…
66decc0 
…twig

Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
@mejiaj mejiaj requested a review from amyleadem January 19, 2024 20:43
amyleadem
amyleadem approved these changes Jan 26, 2024
View reviewed changes
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.

Looks good to me! Thanks @mejiaj

@mahoneycm mahoneycm mentioned this pull request Feb 1, 2024
Closed
5 tasks
mejiaj pushed a commit to uswds/uswds-site that referenced this pull request Feb 20, 2024
Create changelog entry for uswds/uswds#5681.
85a1cab
@mahoneycm mahoneycm mentioned this pull request Feb 21, 2024
Merged
@thisisdano
Copy link
Contributor

Hm. I'm not sure this is something we can recommend. The outcome here — a page with a big ol' sidebar preceding, and blocking, the content is not good. This kind of page would be really bad for the majority of folks accessing a content page on mobile, and would be confusing. (You'd think that you somehow missed the content.)

From the original issue, I think we need to consider option B:

(b) the markup is duplicated and shown/hidden at the appropriate viewport size, but in such a way that ensures the DOM order is always consistent with the visual order.

As is, this is a solution that essentially breaks conventional functionality.

thisisdano
thisisdano suggested changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

See my comment above, but I think this solution has too much of an impact on conventional functionality and reading experience

@amyleadem amyleadem mentioned this pull request Feb 29, 2024
Merged
@mahoneycm mahoneycm mentioned this pull request Mar 1, 2024
Merged
5 tasks
@mahoneycm
Copy link
Contributor

Closing in favor of alternate solution mentioned by @thisisdano #5794

@mahoneycm mahoneycm closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
3 more reviewers

@thisisdano thisisdano thisisdano requested changes

@mahoneycm mahoneycm mahoneycm approved these changes

@amyleadem amyleadem amyleadem 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.

Documentation template tab order should match visual order
4 participants
@mejiaj @mahoneycm @thisisdano @amyleadem