Skip to content
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

Add contribute link to navbar #7274

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Add contribute link to navbar #7274

merged 2 commits into from
Dec 4, 2024

Conversation

bmuenzenmeyer
Copy link
Collaborator

Description

Does the simplest thing we can to improve visibility of contribution, while avoiding some of the problems we encountered with the version support docs.

As @joyeecheung accurately points out, directly linking to this content is the best way to keep it up to date.

Open to iteration, but I wanted to advance the conversation with a concrete implementation.

Validation

See the preview build

image

Related Issues

closes #7237

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@bmuenzenmeyer bmuenzenmeyer requested a review from a team as a code owner November 22, 2024 03:09

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (2)
  • apps/site/navigation.json: Language not supported
  • packages/i18n/locales/en.json: Language not supported
Copy link

vercel bot commented Nov 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Nov 25, 2024 7:03pm

Copy link
Member

@mikeesto mikeesto left a comment

Choose a reason for hiding this comment

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

LGTM, I think this is a good approach given the issues discussed

@bmuenzenmeyer
Copy link
Collaborator Author

todo: we also need a redirect

@bmuenzenmeyer
Copy link
Collaborator Author

@joyeecheung this was your original idea. we now have:

  • top-level contributing link going to the nodejs/node CONTRIBUTING file
  • contributing link in the about/get involved going to the same
  • a redirect in place to the same
  • deleted the duplicate content

can you think of anything else?

Copy link
Contributor

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.79% (631/695) 72.44% (184/254) 94.53% (121/128)

Unit Test Report

Tests Skipped Failures Errors Time
143 0 💤 0 ❌ 0 🔥 5.291s ⏱️

@joyeecheung
Copy link
Member

joyeecheung commented Dec 4, 2024

Thanks for opening the PR! Looks good as far as I can tell, but I do wonder if we should keep the original contribute page and just leave a link in there to CONTRIBUTING.md, in case there are any external websites already linking to the removed page and the URL would be broken (if we think nobody is linking to it then it's fine to just remove, though it seems plausible to me).

@mikeesto
Copy link
Member

mikeesto commented Dec 4, 2024

Thanks for opening the PR! Looks good as far as I can tell, but I do wonder if we should keep the original contribute page and just leave a link in there to CONTRIBUTING.md, in case there are any external websites already linking to the removed page and the URL would be broken (if we think nobody is linking to it then it's fine to just remove, though it seems plausible to me).

That's a good point although I think this redirect should help avoid breaking things: https://github.com/nodejs/nodejs.org/pull/7274/files#diff-5ca5a80a6f81808f1779e8216ad41ca807d50397070d24bc8b4cc72653c84f0bR268-R269

@joyeecheung
Copy link
Member

Oh right I didn't understand what it was doing at first. In that case LGTM, thanks!

@bmuenzenmeyer bmuenzenmeyer merged commit 62ce001 into main Dec 4, 2024
16 checks passed
@bmuenzenmeyer bmuenzenmeyer deleted the contributing branch December 4, 2024 13:12
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.

Add "Contributing" to the navigation bar?
6 participants