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

Global Header: Try unsticking the global header, making local navigation sticky #466

Merged
merged 14 commits into from
Nov 8, 2023

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Oct 2, 2023

See #465, specifically the conversation after this comment. @fcoveram proposed an update to the header where the header does not stick on scroll, but the local nav bar does. There was some discussion and feedback about which bars should stick when. I took @jasmussen's double "I'm into it" comments to mean this is approved for building, and polished up my prototype code.

Since this is a change that at least visually changes the "functionality" of the header, it would need to be announced. I think it could be bundled into the upcoming header & footer changes (cc @ndiego). But we should be sure this is the correct approach, changing it and rolling it back would likely generate negative community feedback.

Screencasts

Showcase - this is the ideal case right now. For the breadcrumbs to be sticky, they need to have "position":{"type":"sticky"}, which has been added with WordPress/wporg-showcase-2022#218

header-showcase.mp4

Next up I click through a few other pages: Home, Download, a download subpage, About, an about subpage, and Documentation — the last one highlights that the other redesign sites will need some adjustments. The same thing happens on the Developers staging site, cc @adamwoodnz

header-main.mp4

Finally, clicking through a classic theme. This doesn't have a local nav bar, so there are no sticky bars here.

header-classic.mp4

To test

This is easiest to test on a sandbox, but if you set it up locally just viewing any site should let you test it.

Try at different screen sizes, various pages, etc.

if ( container ) {
const onScroll = () => {
const { top } = container.getBoundingClientRect();
if ( top <= 32 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably worth a comment on where to find this number or a basic note on how it's calculated.

@StevenDufresne
Copy link
Contributor

👍. I find this much more helpful than a sticky generic header for a site like wp.org..., but that's opinion.

The code approach looks logical, although I admittedly haven't tested it across themes.

Looking at the video, the bottom padding on the sticky subnav that use the paint brush style feels a bit tight. Can we add a few pixels?

Screenshot 2023-10-03 at 12 09 38 PM

@StevenDufresne
Copy link
Contributor

Another comment, but more a future thinking idea....

I love this as a base, but we shouldn't be afraid of sticking a different component if it's usefulness trumps the subnav. I can imagine in the directory archives the filters are more important.

Screenshot 2023-10-03 at 12 25 36 PM

@jasmussen
Copy link
Collaborator

I love this as a base, but we shouldn't be afraid of sticking a different component if it's usefulness trumps the subnav. I can imagine in the directory archives the filters are more important.

I don't disagree, and it's good to know we can flex.

My instincts on filters however go in another direction. Journey-wise, I see the subnav and breadcrumbs as being great actions to have visible always, because you never know when you might be "done" with a particular page and in need of a new destination.

Filters, on the other hand, I perceive to be an intentional step on a journey; you've consumed the homepage, perhaps you've gone to the archive, at which point your choice becomes to either scroll, or filter. And if you scroll and then choose you need to filter after all, then the filter bar was prominent enough at the top that you know to be able to scroll back to it.

Given we can't have too tall of a sticky element, if we were to make the filter bar sticky, we'd want to unstick the subnav and breadcrumb bars. This lends unpredictability that I'm not sure is worth the added accessibility of the filter bar.

I could be wrong! For now, I'm a fan of this, happy to go for it.

@ryelle ryelle force-pushed the try/unsticky-global-sticky-local branch from 45b10bd to 159d585 Compare October 3, 2023 16:48
@ryelle
Copy link
Contributor Author

ryelle commented Oct 3, 2023

Looking at the video, the bottom padding on the sticky subnav that use the paint brush style feels a bit tight. Can we add a few pixels?

Yeah, that's from the cutoff on the brushstroke. How's this?

sticky-brush

we shouldn't be afraid of sticking a different component if it's usefulness trumps the subnav

Technically, that could be done — it would work just like the breadcrumbs, only add "position":{"type":"sticky"} to whatever element should stick (in the child theme templates).

Given we can't have too tall of a sticky element, if we were to make the filter bar sticky, we'd want to unstick the subnav and breadcrumb bars.

The local navigation bar can be unstuck by "position":{"type":"default"} (or any other "type" value, but empty will be the default of sticky).

I could be wrong! For now, I'm a fan of this, happy to go for it.

I do want to reiterate that we should be sure about making this change — doing it and then going back would be a bad experience for the community. Let's talk it through on the call tomorrow.

@jasmussen
Copy link
Collaborator

I didn't mean to suggest I wasn't confident in the change. I definitely am, and happy to talk through it. IMO it's either stick both or stick nothing (with the nuance that if you don't have a breadcrumb bar, of course only stick the secondary bar).

@adamwoodnz adamwoodnz force-pushed the try/unsticky-global-sticky-local branch from 159d585 to c8ea284 Compare November 5, 2023 20:59
@adamwoodnz adamwoodnz force-pushed the try/unsticky-global-sticky-local branch 4 times, most recently from a53e24c to 1dc0679 Compare November 5, 2023 21:53
@adamwoodnz adamwoodnz added the Redesign Related to the wordpress.org redesign project label Nov 5, 2023
@adamwoodnz adamwoodnz added this to the MVP milestone Nov 5, 2023
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.

Tested with Main, Documentation, Showcase and Developer.

Couple of minor changes to the brush stroke variation.

PRs are ready to fix Documentation and Developer.

👍 LGTM

@StevenDufresne
Copy link
Contributor

Do we need to test this in trac?

@adamwoodnz
Copy link
Contributor

Do we need to test this in trac?

Yeah, that's the last thing I thought I'd check

@adamwoodnz adamwoodnz merged commit 826fb7f into trunk Nov 8, 2023
4 checks passed
@adamwoodnz adamwoodnz deleted the try/unsticky-global-sticky-local branch November 8, 2023 02:18
adamwoodnz added a commit that referenced this pull request Nov 8, 2023
adamwoodnz added a commit that referenced this pull request Nov 8, 2023
adamwoodnz added a commit that referenced this pull request Nov 8, 2023
…ng local navigation sticky (#466)" (#494)"

This reverts commit 6c43f89.
adamwoodnz added a commit that referenced this pull request Nov 9, 2023
…ng local navigation sticky (#466)" (#494)" (#495)

This reverts commit 6c43f89.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Header & Footer Redesign Related to the wordpress.org redesign project
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants