Revert ESP related changes #7884
Conversation
This reverts commit 2fc6472.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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. Thank you! |
There was a problem hiding this comment.
Pull Request Overview
This PR reverts previous changes related to ESP and restores earlier versions of styles, translations, and component behaviors. Key changes include removal of modified border styles for buttons, reverting i18n string updates, and restoring prior component logic in release modals and version selection.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/Common/BaseButton/index.module.css | Removed border-related utilities and adjusted background styling for secondary buttons. |
| packages/i18n/locales/en.json | Removed extra guidance in warning messages and reverted translation texts. |
| apps/site/pages/en/index.mdx | Replaced a button group with a simplified structure using WithNodeRelease for download links. |
| apps/site/layouts/layouts.module.css | Adjusted layout max-width and text color to match reverted design. |
| apps/site/layouts/GlowingBackdrop.tsx | Removed an extraneous blank line. |
| apps/site/components/Downloads/ReleaseModal/index.tsx | Reverted rich text warning formatting to a simpler message. |
| apps/site/components/Downloads/Release/VersionDropdown.tsx | Removed redirect-based logic in favor of a simpler onChange handler. |
| apps/site/components/Downloads/Release/ReleaseCodeBox.tsx | Removed an AlertBox for LTS version features to restore previous UI behavior. |
Comments suppressed due to low confidence (8)
packages/i18n/locales/en.json:171
- The removal of the additional guidance (including the link) from the unsupportedVersionWarning message may reduce clarity for end users. Verify that sufficient context is provided elsewhere.
"unsupportedVersionWarning": "This version is out of maintenance. Please use a currently supported version."
apps/site/components/Downloads/ReleaseModal/index.tsx:41
- Switching from a rich text warning (with embedded links) to a plain text message may reduce the clarity of the alert. Confirm that the updated message sufficiently informs users.
{t('components.releaseModal.unsupportedVersionWarning')}
apps/site/components/Downloads/Release/VersionDropdown.tsx:38
- Removing the prior redirection logic from the onChange handler simplifies the component but may affect the intended user navigation flow. Ensure that the new behavior satisfies design expectations.
onChange={setVersion}
apps/site/components/Downloads/Release/ReleaseCodeBox.tsx:127
- The removal of the AlertBox for LTS version features reduces additional UI information. Confirm that critical version details are conveyed elsewhere in the interface if required.
)}
apps/site/pages/en/index.mdx:18
- The update replacing multiple Button components with the WithNodeRelease component may impact the prominence of the call-to-action. Verify that the new layout delivers an equivalent user experience.
<WithNodeRelease status={['LTS']}>
packages/ui-components/Common/BaseButton/index.module.css:80
- The removal of the border classes in the .secondary style may affect visual consistency. Please ensure that this change aligns with overall design requirements and document any necessary style adjustments.
@apply bg-neutral-100
apps/site/layouts/layouts.module.css:39
- Increasing the max-width from 400px to 500px alters the section layout; please confirm that this adjustment is consistent with the overall design system.
max-w-[500px]
apps/site/layouts/layouts.module.css:59
- Changing the text color from text-neutral-600 to text-neutral-800 will affect readability. Please verify that the new color meets accessibility and design standards.
text-neutral-800
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #7884 +/- ##
==========================================
- Coverage 75.49% 75.46% -0.03%
==========================================
Files 101 101
Lines 8311 8311
Branches 218 218
==========================================
- Hits 6274 6272 -2
- Misses 2035 2037 +2
Partials 2 2 ☔ View full report in Codecov by Sentry. |
|
I'll also clarify, there was no block on the PR that landed itself. The folks that landed the PR were under the impression that all objections had been cleared and that by all evidence the appropriate governance process was followed, even tho there does appear to have been a breakdown in communication following. |
|
@jasnell There is a gap in our governance. This is why I propose nodejs/node#58796 |
|
FYI the esp/revert branch contains a revert as well, could you delete that branch if you intend to continue here? |
|
Not a hard block on this, but instead of outright reverting the whole set of changes (including the other design changes to the homepage and downloads components), I think we'd be better off with a branch that just removes the ESP button (if we go down this route). |
|
I'm +1 about removing the ESP button. Yagiz's -1 was unintentionally overlooked, so landing the change was a mistake. Removing it brings us back to stage 1, where we can move forward with whatever plan we decide to follow. I'm having trouble understanding the argument for blocking an attempt to undo a clear mistake. If there are arguments for urgency, the reasoning and consequences should be communicated publicly to minimize misunderstandings. |
|
@RaisinTen the reasons for urgency were clearly communicated to you and the rest of the TSC privately (both in a meeting and in the slack channel) - because they include some sensitive (financial) information. They were clearly communicated as such. We cannot disclose them at this point. We cannot debate another month or just weeks if or not we want to gather the benefits of the ESP program. |
|
I agree with @RaisinTen that may allow us to return to stage 0 and reduce pressure. But apparently we cannot dude to contract restrictions. |
jasnell
dismissed
their stale review
As indicated, I was blocking only long enough to give time for folks to review.
This PR landed while there is an active block by a TSC member and a collaborator. This PR reverts it. #7773
cc @nodejs/tsc