-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
fixes docSideBar #8966
fixes docSideBar #8966
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
3ee0c2a
to
7131a6f
Compare
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.
Didn't have much time to study your solution in depth, but I see layout shifts on React hydration.
Make sure your PR looks good with 3G slow network simulation before/after hydration and with/without scroll
@@ -33,7 +33,7 @@ | |||
.sidebarViewport { | |||
top: 0; | |||
position: sticky; | |||
height: 100%; | |||
max-height: 100vh; | |||
min-height: 277px; |
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.
Not a super fan of magic numbers like these 😅
7131a6f
to
8c427dc
Compare
8c427dc
to
b0e8e86
Compare
Hello @slorber, Currently I am unable to fix the layout shift. I'll keep working on this, but I'm going to close this PR in order to give others the opportunity to create a fix. |
Pre-flight checklist
Motivation
I originally tried to fix this issue a few months back. My original attempts just looked at the CSS strictly. Since then, I have learned a substantial amount about React Hooks, so I decided to take another look at it and was able to come up with a solution.
The Problem:
When the announcement bar is present it takes up
30px
of space at the top of the page. This causes all the elements below it to adjust their position. Previously it appears we attempted to set up a useState, useEffect, and useContext hooks to handle this issue. This hook functionality essentially added amargin-bottom
of30px
whenuseAnnouncementBar
context returned true. This functionality was added to the nav element located atpackages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/Content/index.tsx
. This attempt was close to fixing this issue, but instead we needed to:30px
would work when thescrollY
value was
0
, but as the user scrolled, it needs to adjust dynamically based on currentscrollY
position.The Fix:
In order to fix the issue I:
packages/docusaurus-theme- classic/src/theme/DocSidebar/Desktop/Content/index.tsx
topackages/docusaurus-theme- classic/src/theme/DocSidebar/Desktop/index.tsx.
style
attribute.Note:
When adding the height functionality, some unwanted side effects starting occurring when reducing window size to less than
277px
height. When less than277px
, the sidebarclose button
would start overlapping the other elements in the sidebar. To fix this issue I:close button
to no longer have a position of fixed.min-height
property to ensure that the sidebar would not reduce its size to anything less then 277px. This matches what we currently see in production with one caveat:277px
, the side bar close button becomes less visible and can only being seen by scrolling down on the entire page. This didn't seem ideal, but because the side bar now correctly displays with or without the announcement bar at277px
window height or greater, I figured it was a better solution then whats currently seen in production. If anyone has any suggestions on how we can match how it looks previously from window heights177px - 277px
, that would be awesome.below there is a video and screenshots.
the video shows the solution working on my local computer. This solution can also be viewed on the deployment preview link listed below. I'm not sure how long these links are active, so if it expires, you may have to test it on your local machine.
there are 4 screenshots. Two of them show what the sidebar looks like after this fix, at minimum window height with and without the announcement bar active. The other two show what it looks like currently.
video demonstrating fix:
https://github.com/facebook/docusaurus/assets/62929526/a75e8dcd-c6f0-4608-a401-a20654ca8010
screenshots with fix
screenshots without fix
Test Plan
from your local machine:
yarn start
from the deployment preview:
Note:
When testing if you close the announcement, you can re-enable it by clearing your local browser storage.
Test links
Deploy preview: https://deploy-preview-8966--docusaurus-2.netlify.app/
Related issues/PRs
Issue #8370