-
Notifications
You must be signed in to change notification settings - Fork 292
MNTOR-396: Pre-render locale-specific variants #6427
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
base: main
Are you sure you want to change the base?
Conversation
2f3d706 to
c820646
Compare
ee95a98 to
5eb6dc0
Compare
Vinnl
left a comment
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.
Added some annotations to assist with reviews.
| .container { | ||
| background-color: tokens.$color-white; | ||
| background-image: url("../images/dashboard-top-banner-wave.svg"); | ||
| background-image: url("../../../../../../../assets/dashboard-top-banner-wave.svg"); |
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.
I don't think this should've been necessary, but it looks like the square brackets in the asset URLs broke this. That said, there's probably something else at play as well, because I didn't succeed at creating a minimal reproduction so I could report it upstream.
| children, | ||
| params, | ||
| }: LayoutProps<"/[locale]/user">) { | ||
| const l10nBundles = getL10nBundles(getLangString((await params).locale)); |
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.
This is the meat of this PR - instead of determining the locale from the request headers, we derive it from params.locale, which is set at build time (see generateStaticParams in /src/app/[locale]/layout.tsx).
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.
This one looks like it contains a lot of unrelated changes, but that's because it's a merger of /src/app/(proper_react)/layout.tsx and /src/app/layout.tsx. Since every page that was a child of one was also a child of the other, it didn't really make sense to keep them separate anymore.
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.
With the root layout moved to [locale/layout.tsx, this route was no longer wrapped in a layout. Since we really don't need to load a bunch of session provides just to redirect someone after a logout, I just added a new barebones layout just for this page.
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.
Since we have multiple routes that take a [locale] parameter, I abstracted out the function that generates a list of all the possible values of that param into this file.
| export function getLangString(locale: string): string { | ||
| // Load strings in `locale`, but also the English strings to fall back to | ||
| // for strings that aren't localised to that locale yet: | ||
| return `${locale}, en;q=0.1`; |
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.
fluent-react is built to parse the user's accept-language header, and load up the relevant Fluent bundles based on that. Since, for the website strings, we do this at build time, we don't have a user with an accept-language header; instead, we tell fluent-react that we want the given locale, and English as a fallback.
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.
This file detects when the user visits a route without a locale param and, if so, reads their accept-language header and redirects them to the route with their preferred locale.
| "cloudrun": "npm run db:migrate && npm start", | ||
| "start": "next start", | ||
| "lint": "stylelint '**/*.scss' && prettier --check './src' && eslint --max-warnings=0 . && tsc -p tsconfig.json --noEmit && npm run validate-nimbus", | ||
| "lint": "stylelint '**/*.scss' && prettier --check './src' && eslint --max-warnings=0 . && next typegen && tsc -p tsconfig.json --noEmit && npm run validate-nimbus", |
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.
Next.js recently added the helper types LayoutProps and PageProps that you'll see me use throughout this PR. These automatically provide the right param types for a given route, rather than having to enumerate them manually (with the risk of typos). That said, these helper types only get generated when running npm run dev or npm run build - or with next typegen.
| ], | ||
| // "paths": {}, /* Specify a set of entries that re-map imports to additional lookup locations. */ | ||
| "paths": { | ||
| "@/*": ["./src/*"] |
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.
I used to dislike using path aliases, since they require all tooling to support them (e.g. for go-to-definition on GitHub). However, our not using them has made it really hard to remove e.g. the (proper_react) folder, since apparently it's very hard for all our tools to properly update the imports when moving a lot of files.
Thus, now that I was moving a lot of files around, adding this saved me a lot of time. But if people object, I can try spending some time adding the relevant relative paths.
There are still some places that dynamically determine the user's locale based on request headers (e.g. /api/v1/user/email), but most pages should now be prerendered in a specific locale, and the user should be redirected to those pages when they don't specify the locale in the URL. This also means that users can now be linked to specific locales. Theoretically, this should help both with (localised) search ranking, as well as with performance (which also influences at least Google's ranking). A good addition would be a language switcher, so that users who get linked to a locale that's not theirs, can still switch.
5eb6dc0 to
54b71a3
Compare
References:
Jira: MNTOR-396
Figma:
Description
There are still some places that dynamically determine the user's locale based on request headers (e.g. /api/v1/user/email), but most pages should now be prerendered in a specific locale, and the user should be redirected to those pages when they don't specify the locale in the URL. This also means that users can now be linked to specific locales.
Theoretically, this should help both with (localised) search ranking, as well as with performance (which also influences at least Google's ranking).
A good addition would be a language switcher, so that users who get linked to a locale that's not theirs, can still switch.
How to test
All functionality should continue to work. If you visit existing URLs, you should automatically get redirected to the same URL with your preferred locale prefixed.
The main change would be that your browser's specified fallback languages aren't taken into account, i.e. if your Accept Language specifies Dutch, German, you will get Dutch strings, but missing strings will be replaced by English ones. This is because the strings are pre-rendered at build time, and thus no browser settings are known. At runtime, the browser settings are then only used to determine which locale to redirect to.
Checklist (Definition of Done)