-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix(ui): Sidebar scroll not showing last element #8563
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
Open
malav2110
wants to merge
6
commits into
nodejs:main
Choose a base branch
from
malav2110:fix-sidebar-scroll-last-item
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+4
−2
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6289850
fix(ui): Sidebar scroll not showing last element
malav2110 e7054d2
fix(ui): correct sidebar sticky positioning for better visibility
malav2110 8bb2784
Merge branch 'nodejs:main' into fix-sidebar-scroll-last-item
malav2110 8494756
fix: update ui-package version to 1.5.9 in package.json
malav2110 d84fcd3
fix(ui): adjust sidebar styles for improved layout and scrolling beha…
malav2110 5502c79
Merge branch 'nodejs:main' into fix-sidebar-scroll-last-item
malav2110 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| py-6 | ||
| sm:overflow-auto | ||
| sm:border-r | ||
| sm:pb-16 | ||
| md:max-w-xs | ||
| lg:px-6 | ||
| dark:border-neutral-900 | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Padding could probably be a bit smaller.
Uh oh!
There was an error while loading. Please reload this page.
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.
With
pb-14, the last item on the list is barely visible thus addedpb-16.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.
Is that really the case? This is how much padding I see
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.
You're probably referring to the padding when the footer isn't visible.
So this is probably not the place you want to change the padding.
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.
For example, if you see the beginning of the sidebar has the same margins/paddings no matter if the header is visible or not
But the same isn't truthy for the end of the sidebar:
So this means this place is probably not the right one to add padding. Or it must be a mix of two different places.
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.
That's weird. the Metabar is also stick and doesn't have said issue, so I'm pretty sure the same can be accomplished to the Sidebar 🤔
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.
I did review the MetaBar to compare.
It is also
position: stickywithtop: 0, but the key difference is that it does not have a height restriction or internal scrolling. Its content is short enough to fit naturally in the viewport.The SideBar is different. On Learn page, it has many more items, so it needs
max-height+overflow-y-autoto scroll internally.That's where the issue comes from:
100vhuses the full viewport height, but when the NavBar (+ security banner) is visible, it takes up space at top. The Sidebar still assumes the full height, so the last item/s ends up getting clipped.I tested both cases:
Because the MetaBar rarely overflows, it avoids this edge case entirely.
Uh oh!
There was an error while loading. Please reload this page.
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.
Unfortunately, you're wrong. Example: https://nodejs-api-docs-tooling.vercel.app/child_process.html / https://nodejs-api-docs-tooling.vercel.app/globals.html
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.
(I would recommend to avoid making assumptions, if I tell you it is possible or something the contrary, then I'm pretty sure it is possible 😛)
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.
(I just want to avoid giving you a solution plated, so you can try it yourself 👀)