-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
fix(esp): add utm params, don't use prohibited strings #7881
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes naming issues by removing prohibited strings referencing "Node" and adds UTM parameters to the support URL.
- Removes the inappropriate "on Node!" text from feature notices in locale files.
- Adds UTM parameters to the security support button URL in the MDX page.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/i18n/locales/en.json | Removed "on Node!" from ltsVersionFeaturesNotice to ensure correct naming conventions. |
apps/site/pages/en/index.mdx | Updated the support URL with UTM parameters for tracking purposes. |
Comments suppressed due to low confidence (2)
packages/i18n/locales/en.json:171
- Confirm that removing 'on Node!' here satisfies our naming and brand guidelines and maintains clarity in messaging.
"unsupportedVersionWarning": "This version is out of maintenance. Please use a currently supported version. <link>Understand EOL support.</link>",
apps/site/pages/en/index.mdx:24
- [nitpick] Verify that the 'NodeJS+' UTM parameter meets our naming conventions and that the encoding is as intended for tracking purposes.
<Button kind="secondary" className="!block" href="https://www.herodevs.com/support/node-nes?utm_source=NodeJS+&utm_medium=Link&utm_campaign=Homepage_button">
Lighthouse Results
|
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 #7881 +/- ##
==========================================
- Coverage 75.47% 75.45% -0.03%
==========================================
Files 101 101
Lines 8311 8311
Branches 218 218
==========================================
- Hits 6273 6271 -2
- Misses 2036 2038 +2
Partials 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't set any utm tags prior approval.
We've been asked to add UTM tags in the original issue, which received no objections. Why wouldn't we add them to a new link we are adding to increase traffic to the source? |
@ovflowd in the meeting we were explicitly asked to include UTMs so that the traffic can be attributed to Node.js/the foundation... I understand that we haven't been told exactly what those params should be, but I think given that we've decided to land this without hearing back, we can make a very educated guess based on the params used in the ESP blog. |
If you are strictly against adding UTMs, then I will ask that we revert adding the ESP to the homepage until we hear back, so that we're not sabotaging our (and the foundation's) participation in the ESP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to fast-track this PR. I honestly don't have the emotional energy to argue or not on this matter. Do what y'all believe should be the right course of action.
cc @nodejs/nodejs-website feels like this possibly should be fast-tracled? |
I think this can merge immediately without fast-track, as a urgent bugfix/errata? But if not, yeh, let's fast-track please. |
Merging now as an errata fix |
Unless instructed otherwise, we should assume that UTM parameters are required. See https://openjs-foundation.slack.com/archives/CVAMEJ4UV/p1750546862196709?thread_ts=1750198935.143539&cid=CVAMEJ4UV and onward.
The presence of prohibited strings on the main download page suggests that users should refer to Node.js as "Node," which is incorrect. Users should use "Node.js" when referring to the project, and "node" when referring to the executable.