Skip to content
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

Local Navigation: Improve style and behavior across screen sizes #573

Merged
merged 21 commits into from
Feb 19, 2024

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Feb 12, 2024

This PR implements the style & behavior changes described in #535. The biggest change is that the navigation will now switch to the collapsed (mobile) view if it does not have enough space to stay on one single line (fixes WordPress/wporg-documentation-2022#80).

I did that by dropping in a second menu automatically - a filter is added to any nav blocks with wporg/local-navigation-bar as a parent block. That filter (add_extra_navigation) runs over the render_block HTML, so it can use all the same block attributes to create a second wp:navigation which is always in the collapsed (mobile) state. It's also given a special class name, so that it can be hidden/made visible by CSS. This ensures only one menu is visible/accessible at a time. In the javascript, a function is added to resize to check the available width against the section title & navigation items, and toggles the appropriate class based on what fits better.

Using a second menu like this also avoids trying to work around the current navigation menu toggle code, which has changed dramatically in the past.

Since the "mobile" view now shows on non-mobile screen sizes, the design was also updated and the styles here switch to a dropdown style when this version is shown at >600px wide.

There are also style changes to update focus styles & minor spacing fixes.

Screenshots

Documentation, using the page from WordPress/wporg-documentation-2022#80:

Default Open menu Mobile menu

The bottom border should only show when scrolling:

about-section-scroll.mp4

Here's an example of the menu collapsing when it would have wrapped (plus the "L3 header" variant updates):
https://github.com/WordPress/wporg-mu-plugins/assets/541093/71c069fd-9a7c-4c28-9e27-50c385faa286

Some long page titles still overflow the space available even with the collapsed nav menu. I've just set them to overflow:hidden, and at 550px wide, the page title is hidden.

Full title visible Overflow kicks in Small screen, title hidden

To test

Check out this PR and use it with any site, it doesn't require any local changes.

I'll have a separate PR for Developer to implement the different page level header templates, which was used to capture some of these screenshots.

@adamwoodnz
Copy link
Contributor

adamwoodnz commented Feb 13, 2024

Looking great, still testing, but found a broken width on Developer, looks like padding causing the wrapping

@adamwoodnz
Copy link
Contributor

Seems unnecessary to me to show the title on the homepage on tablet and mobile, especially as the local nav isn't sticky on those screen sizes, so it isn't visible once scrolled

Tablet Mobile Scrolled
developer wordpress org_(iPad) developer wordpress org_(Samsung Galaxy S20 Ultra) developer wordpress org_(Samsung Galaxy S20 Ultra) (1)

@adamwoodnz
Copy link
Contributor

From #535

For level 3 and deeper pages, page name is hidden as breadcrumb takes that role. But once you scroll down, all pages, except for the section’s landing pages, show the page name.

I think we're missing this behaviour, for me the page name is visible in the local nav before scrolling

local.nav.scroll.mp4

Copy link
Contributor

@adamwoodnz adamwoodnz 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 great 👍

Fixes the Docs bug for me and nails most of the new behaviour. I've left comments for things I've found but none are blockers imo.

@ryelle
Copy link
Contributor Author

ryelle commented Feb 14, 2024

I think we're missing this behaviour, for me the page name is visible in the local nav before scrolling

I thought the animation of both the W and the page title was a little strange, so I'm waiting for some feedback on the main issue (see the comment here).

Seems unnecessary to me to show the title on the homepage on tablet and mobile, especially as the local nav isn't sticky on those screen sizes, so it isn't visible once scrolled

Could you bring that up on the main issue? #535

@ryelle
Copy link
Contributor Author

ryelle commented Feb 16, 2024

Looking great, still testing, but found a broken width on Developer, looks like padding causing the wrapping

Fixed with bba3713

I think we're missing this behaviour, for me the page name is visible in the local nav before scrolling

I've added a class to fade-in the page title, and updated developer to use it. It'll look like this:

fade-in-page.mp4

I think that addresses all the feedback here, but let me know if I missed something.

Copy link
Contributor

@renintw renintw left a comment

Choose a reason for hiding this comment

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

Tested and looks good 👍

@ryelle ryelle merged commit 43dc4ec into trunk Feb 19, 2024
2 checks passed
@ryelle ryelle deleted the update/local-subnav branch February 19, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Local nav doesn't fit on vertical tablet sized screens
4 participants