-
Notifications
You must be signed in to change notification settings - Fork 553
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
Sticky breadcrumb and sidebar navs #1198
Sticky breadcrumb and sidebar navs #1198
Conversation
Both the breadcrumb (parent) and sidebar (sibling) navs provide key context while reading the page. Making them sticky (and optionally giving the sidebar nav its own scroll context) makes their context available to the whole page, not just the start.
🚀 Preview for commit 2cf9118 can be found at https://web-php-pr-1198.preview.thephp.foundation |
🚀 Regression report for commit 2cf9118 is at https://web-php-regression-report-pr-1198.preview.thephp.foundation |
All the failed tests (with the possible exception of the archives page) appear to be because the rounded corner does not match the screenshots. This is expected behavior. The sticky breadcrumb nav makes it slightly harder to tell when the page top has been reached, so I added a single round corner to signal the content's upper limit. |
Hi, thanks for the PR! I've updated the snapshots so they don't fall off due to the wrong year, Is this rounding due to UI practices, or is there some technical issue? If the former, I'm not good at it, so I can't judge (but to me a right angle or a rounded one fits equally), If the second, we can update the snapshots for tests, including rounding. |
The rounded corner is to preemptively solve a minor UX concern introduced by the sticky layout, not to solve a technical issue. I'd be happy to remove that line from my branch if it is a sticking point (no pun intended) or if no one else sees the concern. |
All right! Let's wait for feedback on the rounding BTW, do you think maybe adding sticky to similar pages as well? |
The way I see it, the other pages are split between a maybe-so and a probably-not.
|
Indeed, let's postpone this issue for the future :) So, LGTM, just need to either remove rounding, or update snapshots. |
Resetting top border radius to zero to pass tests php#1198
I'll take that as a "no" (or at least a "not yet") on the border radius. I rolled back that change and pulled in the latest version of the master branch. |
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.
Thanks a lot!
Both the breadcrumb (parent) and sidebar (sibling) navs provide key context while reading the page. Making them sticky (and optionally giving the sidebar nav its own scroll context) makes their context available to the whole page, not just the start.
close #1197