-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
style: update sidebar item spacing and scrollbar styles #8166
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull Request Overview
Enhances the Learn page sidebar UI by improving item spacing and introduces consistent custom scrollbar styling across the website to align with the theme colors.
- Updates sidebar item padding and progression group spacing for better visual hierarchy
- Implements custom scrollbar styling with green theme colors for visual consistency
- Fixes dark mode display issues in progression group connectors
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/ui-components/src/styles/index.css | Adds global custom scrollbar styling with green theme colors |
packages/ui-components/src/Containers/Sidebar/SidebarItem/index.module.css | Adds left padding to sidebar items for improved spacing |
packages/ui-components/src/Containers/Sidebar/SidebarGroup/index.module.css | Improves progression group spacing and fixes dark mode connector visibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Islem Djoudi <[email protected]>
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 for your observation , It has been fixed ✅ |
Lighthouse Results
|
@@ -37,3 +37,17 @@ | |||
} | |||
|
|||
@custom-variant aria-current (&[aria-current="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.
@canerakdas cam tailwind styles be used here?
Also, I wonder if we really want to change the scrollbar on this PR. I feel this shouldn't be done on this PR and done in another discussion.
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.
@canerakdas cam tailwind styles be used here?
It should be available, even if these utility classes are not available in tailwind, they can be added like
@apply [scrollbar-width:thin]
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8166 +/- ##
=======================================
Coverage 76.59% 76.59%
=======================================
Files 115 115
Lines 9602 9602
Branches 322 322
=======================================
Hits 7355 7355
Misses 2246 2246
Partials 1 1 ☔ View full report in Codecov by Sentry. |
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.
This PR is doing 3 different things at the same time that need different agreement:
- Sidebar spacing
- Colours
- Scrollbar styles
All of that are quite subjective and I'd like to hear from the rest of the team before we get this merged. cc @nodejs/nodejs-website
I think these are topics that should be evaluated separately. Although improves/fixes the styling of the progression sidebar, updating the scroll bars across the entire site is an little bit extreme update in my opinion |
In the design system, we already have the green hexagon for the active state (https://www.figma.com/design/a10cjjw3MzvRQMPT9FP3xz/Node.js?node-id=332-34179&t=DqAOj6HcLm6dUhkN-0) I still believe that these links shouldn’t have hover or active backgrounds. The design represents progress a flow from one node to another connected by lines. Adding a background here breaks that flow in my opinion 🥲 I remember we discussed similar points earlier regarding the links in the header. cc @nodejs/nodejs-website |
Description
Validation
before :


After :



Related Issues
Check List
pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.