fix: RSS button navigate to 404 page if not with en locale #8073
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -21,6 +21,7 @@ const BlogHeader: FC<BlogHeaderProps> = ({ category }) => { | |||
| {t('layouts.blog.title')} | |||
| <Link | |||
| href={`/feed/${currentFile}`} | |||
| {...{ locale: 'en' }} // RSS feeds only exist in English | |||
There was a problem hiding this comment.
| {...{ locale: 'en' }} // RSS feeds only exist in English | |
| locale='en' // RSS feeds only exist in English |
There was a problem hiding this comment.
@avivkeller
I used object spread here to pass locale because our custom Link component’s prop types are based on HTMLAnchorElement and don’t include locale, thus will cause type error. This approach avoids a larger refactor while keeping the behavior consistent with how the locale prop would be passed in a properly typed Next.js Link. It also ensures we don’t introduce unnecessary complexity just to satisfy type checking for a value that is already fixed (en).
There was a problem hiding this comment.
I'd rather perform the larger change than have a hacky solution.
There was a problem hiding this comment.
I'll fix it in a PR right now
There was a problem hiding this comment.
I can also do that change in my day time tomorrow.
I will see if there is an updated one tomorrow, or else I can help the change.
Thanks for the feedback @avivkeller
|
@AugustinMauroy @avivkeller |
We have added feeds with translated links in case anyone wants to translate the blog posts. At the moment, this is not being done because we are struggling to get a decent translation for the limited amount of content |
|
@AugustinMauroy |
|
@AugustinMauroy |
github-actions
bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8073 +/- ##
==========================================
- Coverage 72.80% 72.76% -0.04%
==========================================
Files 96 96
Lines 8328 8328
Branches 214 215 +1
==========================================
- Hits 6063 6060 -3
- Misses 2264 2267 +3
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
@AugustinMauroy |
Don't worry about that |
|
@ovflowd @mikeesto @avivkeller |
I don't have a huge preference either way, but I'd prefer to, contrary to what Augustin said, not do this in the routing side, but rather on the linking side, since we only need to adjust a broken link, not add several more files to the site. We only have an English RSS feed, not one for each language. Opinions, @nodejs/nodejs-website? |
|
I think this also depends on whether you plan to support multiple languages for the RSS feed in the future. From a user’s perspective, if we see that the URL for a feed is in the However, if there is a plan to support multiple languages for the RSS feed, then making the change on the routing side seems reasonable, as it would prepare for future translation work. |
|
@avivkeller @ovflowd @AugustinMauroy |
Description
close #8070
RSS feed links on non-English pages redirect to English feeds because RSS feeds only exist in English.
It is more user friendly instead of showing 404 page without any explanation to the user.
Validation
To test this fix:
pnpm dev/ja/blog)/en/feed/blog.xmlor/en/feed/${file}.xmlif you are in certain feed type) instead of 404 not found pageRelated Issues
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.