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

Esp participation #7086

Merged
merged 11 commits into from
Oct 18, 2024
Merged

Esp participation #7086

merged 11 commits into from
Oct 18, 2024

Conversation

joeeames
Copy link
Contributor

@joeeames joeeames commented Oct 3, 2024

Description

This PR adds links and adjustments for participation in the OpenJS Ecosystem Sustainability Program (ESP). This includes a new top nav link, a warning on the download page when downloading EOL versions, and information on the "previous releases" page including a link to HeroDevs.

Once merged, the graphic on the Previous Releases page will be updated, and the core node.js README on the github repo will be updated to summarize some of this information. This will be done with separate PR's.

Validation

  1. New top nav pointing to the Previous Releases page
  2. a warning box on the download page when selecting a version before 18
  3. new text on the Previous Releases page

Related Issues

Check List

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

@joeeames joeeames requested a review from a team as a code owner October 3, 2024 22:51
Copy link

vercel bot commented Oct 3, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Oct 18, 2024 2:12pm

Copy link

github-actions bot commented Oct 3, 2024

Note

Your Pull Request seems to be updating Translations of the Node.js Website.

Whilst we appreciate your intent; Any Translation update should be done through our Crowdin Project.
We recommend giving a read on our Translation Guidelines.

Thank you!

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.

It's unclear to me how this fits in with #7037 ? If the underlying motivation is to market support for EOL versions through HeroDevs, that can be a short spiel on the Node.js Releases page. The changes to the Download page seem unproductive given it's about to be redesigned

apps/site/components/Downloads/Release/ReleaseCodeBox.tsx Outdated Show resolved Hide resolved
apps/site/navigation.json Outdated Show resolved Hide resolved
packages/i18n/locales/fr.json Outdated Show resolved Hide resolved
Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

+1 for this mention; +1 to display it as "alert box"

@ovflowd
Copy link
Member

ovflowd commented Oct 6, 2024

[ X] I have read the Contributing Guidelines and made commit messages that follow the guideline.

Have you actually read the guidelines? 🤔 -- because some of the changes you've done here, such as translations are mentioned there 🤔

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Overall I'm fine with the change, although not ideal, but you definitely need to use the existing components from our design system please.

@AugustinMauroy
Copy link
Contributor

but you definitely need to use the existing components from our design system please.

I had the same feeling. But we don't have any red flash components at the moment, with the exception of Banner. If not there's Stability but that's an API component that hasn't been implemented yet.

@ovflowd
Copy link
Member

ovflowd commented Oct 6, 2024

but you definitely need to use the existing components from our design system please.

I had the same feeling. But we don't have any red flash components at the moment, with the exception of Banner. If not there's Stability but that's an API component that hasn't been implemented yet.

Banner component is fine.

@joeeames
Copy link
Contributor Author

joeeames commented Oct 8, 2024

thanks for all the review. I apologize for what I missed. Although I did read the contributing guidelines @ovflowd I did misunderstand the translation piece. I'll get changes made here and resubmitted.

@AugustinMauroy
Copy link
Contributor

I'm -1 to have a new entry on nav bar because it's not 100% usefull. It also introduces a strange effect on the navbar because it hasn't been designed to support two entries that have the same ‘section’.

Capture d’écran 2024-10-15 à 09 26 12

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

Some thing can be clean

apps/site/components/Downloads/Release/ReleaseCodeBox.tsx Outdated Show resolved Hide resolved
apps/site/components/Downloads/Release/ReleaseCodeBox.tsx Outdated Show resolved Hide resolved
@joeeames
Copy link
Contributor Author

changes have been made to address all requested items (I believe)

@joeeames
Copy link
Contributor Author

I'm -1 to have a new entry on nav bar because it's not 100% usefull. It also introduces a strange effect on the navbar because it hasn't been designed to support two entries that have the same ‘section’.

Capture d’écran 2024-10-15 à 09 26 12

We need to find a resolution here. Participation in the ESP program is about making sure that users who need to understand what versions are supported and what are not, and consequently find out about solutions for versions that are not still supported, need a simple and easy way to find the page documenting this information. Currently it's buried under "about" and then "previous releases" which isn't at all straightforward for someone wanting to find out about support. I get the comment about it being "100% useful" but for someone trying to find out if the version they're on is supported, these links are critical.

Some other options are to create an entirely new page as we've done with many other projects, but the previous releases page is by far more suited for this. Or to remove it from the about section and put it in its own section? We're open to anything that fits the criteria of "easy to find". So any ideas are welcome here.

@AugustinMauroy
Copy link
Contributor

I don't agree with you that it's buried in the about section. It's fine like that. Because the section above is a navbar whose purpose is to navigate between sections and NOT to go to a specific article.
I've looked elsewhere and no-one puts ‘version support’ in the nav. I've checked for Deno, .net, java, php, ruby.

@joeeames
Copy link
Contributor Author

I don't agree with you that it's buried in the about section. It's fine like that. Because the section above is a navbar whose purpose is to navigate between sections and NOT to go to a specific article. I've looked elsewhere and no-one puts ‘version support’ in the nav. I've checked for Deno, .net, java, php, ruby.

Sorry I wasn't meaning "projects in general" but "projects we've worked with". Meaning the purpose of the ESP is to improve the use case for users for whom support matters. Part of that is making the support information easy to find. So to clarify, on the other projects that HeroDevs has worked with (angular, jquery, bootstrap, vue 2, ESLint, etc) we've added links to this information usually in the main nav or on the home page. This is obviously not the only way to do this, but we work to make this page easy to find.

It's very subjective of course, I just try to put myself in the shoes of someone who may not know a site intimately, can I find the information I'm looking for and if not what change would make it easy to find. Since I personally do this as I look to see what changes to recommend to make, I think I'm a good proxy for this user profile. So when I went looking for the information on the Node website I definitely had some trouble finding it. Again, this is very subjective.

Thought I'd throw in some screenshots of ways some other project have addressed this. Two of these three did NOT go with a new top nav item but instead added something to the home page.

ESLINT:
image

jQuery:
image

Bootstrap:
image

@marco-ippolito
Copy link
Member

@mikeesto can you please take a look again? Please let us know how to move forward

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@bmuenzenmeyer
Copy link
Collaborator

I'm unsure why #7037 was first referenced, the OP doesn't seem to.

To be clear - the project and website maintainers fully support this effort. I'm happy to help land it to satisfaction of all parties.
I briefly looked into the rendering concerns that Augustin (rightly) mentioned within this comment - I think that's the only real blocker from a UX perspective.

I'll try to move things along by end of weekend.

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.

Thanks for taking our feedback on board @joeeames . Perhaps the nav bar item could be named "Releases" instead of "Version Support" ? I agree with others that it would be good to fix the UX here too before this lands

Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

push a commit onto the branch, hope that's okay with you @joeeames

apps/site/next.constants.mjs Show resolved Hide resolved
@bmuenzenmeyer bmuenzenmeyer added the github_actions:pull-request Trigger Pull Request Checks label Oct 18, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Oct 18, 2024
Copy link

github-actions bot commented Oct 18, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 100 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 100 🟢 92 🔗

@bmuenzenmeyer
Copy link
Collaborator

failing test is resolved on main via 851b085

Copy link

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.54% (594/656) 76.27% (180/236) 94.35% (117/124)

Unit Test Report

Tests Skipped Failures Errors Time
134 0 💤 0 ❌ 0 🔥 5.216s ⏱️

@bmuenzenmeyer bmuenzenmeyer added this pull request to the merge queue Oct 18, 2024
Merged via the queue into nodejs:main with commit 98b9e42 Oct 18, 2024
15 checks passed
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.

7 participants