Skip to content

adds beta docs to learn - #114

Open
bmuenzenmeyer wants to merge 6 commits into
mainfrom
learn-beta-docs
Open

adds beta docs to learn#114
bmuenzenmeyer wants to merge 6 commits into
mainfrom
learn-beta-docs

Conversation

@bmuenzenmeyer

@bmuenzenmeyer bmuenzenmeyer commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Need to see if we need to update ui-components or any other aspect of what made the nav accent.
cross-ref nodejs/nodejs.org#8998

Copilot AI review requested due to automatic review settings July 30, 2026 22:22
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nodejs-learn Ready Ready Preview Aug 3, 2026 1:31pm

Request Review

@github-actions

Copy link
Copy Markdown

👋 Codeowner Review Request

The following codeowners have been identified for the changed files:

Team reviewers: @nodejs/nodejs-website

Please review the changes when you have a chance. Thank you! 🙏

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The new NavBar entry uses a label i18n key that is not defined/used elsewhere in this repo and is inconsistent with existing text fields, likely causing an incorrect or missing link label.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR adds a new top-level navigation link to the Node.js Beta documentation site in site.json, intended to surface “Beta Docs” prominently in the Learn site NavBar.

Changes:

  • Add a new NavBar item linking to https://beta.docs.nodejs.org/
  • Mark the new link as opening in a new tab and visually accented
File summaries
File Description
site.json Adds a new navigation entry for the Beta docs site in the main NavBar list.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread site.json

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The new nav item uses label instead of text, which is likely to render the link with no visible text given how navItems are passed through.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

site.json:21

  • This new nav entry uses label, but the rest of site.json navigation items use text, and components/Navigation/index.jsx passes navItems={navigation} directly into @node-core/ui-components NavBar (no label->text mapping). As a result, this item will likely render with an empty label. Use text (or add mapping code, but that would be a separate change).
      "label": "components.containers.navBar.links.betaDocs",
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The new nav entry uses label instead of text, which likely prevents the Beta Docs link text from rendering with the current NavBar integration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

site.json:23

  • components/Navigation passes navigation directly into @node-core/ui-components/Containers/NavBar, which expects each item to provide a text field (as the other entries in site.json do). Using label here will likely result in the Beta Docs item rendering without any visible text (and the i18n key isn’t resolved in this repo).
      "link": "https://beta.docs.nodejs.org/",
      "label": "components.containers.navBar.links.betaDocs",
      "target": "_blank",
      "accent": true
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread site.json Outdated
@bmuenzenmeyer

Copy link
Copy Markdown
Contributor Author

this repo specifically i have all kinds of janky lockfile problems with.

Copilot AI previously approved these changes Jul 31, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The changes are small, consistent with existing navigation patterns, and the dependency + lockfile updates align with the new nav behavior.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings July 31, 2026 19:15
Copilot AI dismissed their stale review, a newer Copilot review was requested July 31, 2026 19:20
Copilot AI previously approved these changes Jul 31, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The change is small, aligns with the referenced upstream implementation, and the dependency bump matches the new nav item shape.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@bmuenzenmeyer

Copy link
Copy Markdown
Contributor Author

@avivkeller these lint failures may be worrisome. the thinks are working in production so there might be some incompatibility in the rule

@avivkeller

Copy link
Copy Markdown
Member

the thinks are working in production so there might be some incompatibility in the rule

Look at https://github.com/nodejs/nodejs.org/blob/main/apps/site/.remarkrc.json. I probably need to allow for mapping /learn as the root

@avivkeller

Copy link
Copy Markdown
Member

That being said, they are just warnings (since they can be slightly inaccurate, as you can see) and can be ignored, the error is from prettier

@bmuenzenmeyer

Copy link
Copy Markdown
Contributor Author

I see that now with prettier - oddly still, my prettier output is clean.

On branch learn-beta-docs
Your branch is up to date with 'origin/learn-beta-docs'.

nothing to commit, working tree clean
➜  learn git:(learn-beta-docs) npm run format:check

> format:check
> prettier --check .

Checking formatting...
All matched files use Prettier code style!

@bmuenzenmeyer
bmuenzenmeyer marked this pull request as ready for review August 2, 2026 21:41
@bmuenzenmeyer
bmuenzenmeyer requested a review from a team as a code owner August 2, 2026 21:41
@cursor

cursor Bot commented Aug 2, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Navigation and dependency version bump only; no application logic or security-sensitive changes in this repo.

Overview
Adds a Beta Docs entry to the site navigation in site.json, linking to https://beta.docs.nodejs.org/ in a new tab with accent: true so it stands out like the main nodejs.org nav.

Bumps @node-core/ui-components from ^1.7.3 to ^1.7.4 in package.json (with a large package-lock.json refresh from that bump and related transitive updates).

Reviewed by Cursor Bugbot for commit f2724ee. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread site.json
"link": "/blog",
"text": "Blog"
},
{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is wrong compared to nodejs.org, should be after the old docs entry

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch addressed via f2724ee

Copilot AI review requested due to automatic review settings August 3, 2026 13:31
Copilot AI dismissed their stale review, a newer Copilot review was requested August 3, 2026 13:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The changes are small, consistent with existing navigation configuration, and the dependency bump aligns with the new nav item’s usage.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details
  • Files reviewed: 2/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants