Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #745 +/- ##
==========================================
+ Coverage 78.42% 78.75% +0.32%
==========================================
Files 157 159 +2
Lines 13959 14151 +192
Branches 1152 1177 +25
==========================================
+ Hits 10948 11144 +196
+ Misses 3006 3003 -3
+ Partials 5 4 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the web generator’s sidebar navigation by introducing grouped sidebar sections (via per-page categories with a temporary static fallback for Node.js docs), and adjusts the version selector styling/variant to align the sidebar UI.
Changes:
- Extend
#theme/configpagesentries to include an optionalcategoryfield and update related typing/tests. - Add a sidebar grouping utility (with a temporary
SIDEBAR_GROUPS-based fallback) and wire it into theSideBarUI. - Add a global CSS override to adjust the version select min-width.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/generators/web/utils/config.mjs | Emits pages tuples with an added optional category value. |
| src/generators/web/utils/tests/config.test.mjs | Updates tests/fixtures to validate the new pages tuple shape. |
| src/generators/web/ui/utils/sidebar.mjs | New grouping logic (category + fallback mapping) for sidebar navigation. |
| src/generators/web/ui/utils/tests/sidebar.test.mjs | Adds unit tests for sidebar grouping behavior. |
| src/generators/web/ui/types.d.ts | Updates #theme/config typings for the new pages tuple. |
| src/generators/web/ui/index.css | Adds global CSS override for the select combobox min-width. |
| src/generators/web/ui/components/SideBar/index.jsx | Uses grouped sidebar data and switches the version select away from the inline variant. |
| src/generators/web/constants.mjs | Adds temporary static SIDEBAR_GROUPS definition for Node.js docs grouping fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tooling into feat/sidebar-groups
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
We shouldn't hard code groups at all. Simply, add the logic for supporting frontmatter, accept that after this gets merged it won't work, but then upstream changes on nodejs/node to then add the proper groups to each file.
|
Hypothetically, as noted in #747, if we allow (as that PR does) users to add custom config options, which are passed to |
PR SummaryMedium Risk Overview Updates the built-in Introduces Reviewed by Cursor Bugbot for commit 4e2ad68. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
cc @canerakdas seems like it needs rebasing again? |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4e2ad68. Configure here.
| * Redirect to a URL | ||
| * @param {string} url URL | ||
| */ | ||
| export const redirect = url => (window.location.href = url); |
There was a problem hiding this comment.
I wonder if window ref should be done directly, on the server side this would bork-ish.
There was a problem hiding this comment.
Also this seems generic enough it shouldn't really be a fn IDK
| * @param {string} url URL | ||
| */ | ||
| const redirect = url => (window.location.href = url); | ||
| import styles from './index.module.css'; |
There was a problem hiding this comment.
style imports should happen after everything afaik
| export const pages: Array<[string, string]>; | ||
| export const languageDisplayNameMap: Map<string[], string>; | ||
| export const pages: Array< | ||
| [number, { heading: string; path: string; category?: string }] |
There was a problem hiding this comment.
Can { heading: string; path: string; category?: string } be its own type?
| /** | ||
| * Default weight used when a page does not define one. | ||
| */ | ||
| export const DEFAULT_PAGE_WEIGHT = -1; |
There was a problem hiding this comment.
I don't think we should have a page weight system at all. I chatted that with @avivkeller and ordering shouldn't be done via a page weight.
Afaik, what I chatted with @avivkeller is that navigation lists should be manual entries.
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
I wonder if this PR is still necessary, now that @avivkeller made a fix already? The fix being that navigation entries are defined on doc-kit config, I think you can sync with @avivkeller what I agreed with him, and maybe if more docs are needed he can fill you in?
But on github.com/nodejs/learn this is at least fixed already?
Description
This PR's main goal is to group the links in the sidebar to help developers find the content they're looking for more easily.
As a temporary solution, grouping for the Node.js docs is done using a static array. However, in the long term, we plan to add category information to pages in YAML format and remove this array as well
Validation
Before / After
Related Issues
Related to #646, nodejs/learn#6
Check List
node --run testand all tests passed.node --run format&node --run lint.