-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[docs] Disable translations #34820
[docs] Disable translations #34820
Conversation
|
docs/pages/_app.js
Outdated
@@ -39,39 +32,6 @@ const clientSideEmotionCache = createEmotionCache(); | |||
// Set the locales that the documentation automatically redirects to. | |||
acceptLanguage.languages(LANGUAGES); | |||
|
|||
function LanguageNegotiation() { |
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 should do the same in MUI X and synchronize docs deployment to production to avoid redirect loop for users that previously selected language different than en
.
Here's what happens on preview deployment when user has userLanguage=pt
cookie:
Screen.Recording.2022-10-19.at.11.17.20.mov
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'll open a PR
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.
Nice catch, let's keep this in mind.
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.
@@ -4,10 +4,10 @@ const CODE_VARIANTS = { | |||
}; | |||
|
|||
// Valid languages to server-side render in production | |||
const LANGUAGES = ['en', 'zh', 'pt']; | |||
const LANGUAGES = ['en']; |
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.
Do we need these constants? They're used in next.config.js
, but maybe we should remove translation logic there as well?
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 discussed that we want to keep the infrastructure around the translations so that we can easily add it back if we decide in the future, this is why I decided to left these constants and not do any changes in the next.config.js
, but I don't mind removing the logic there as well. cc @siriwatknp what do you think?
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.
Let's remove them too. There are many places that use LANGUAGES
, so I think let's keep them there, otherwise it will be a lot more work. The changes are sufficient.
docs/public/_redirects
Outdated
/pt/* /:splat 301 | ||
/zh/* /:splat 301 |
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.
/pt/* /:splat 301 | |
/zh/* /:splat 301 | |
/pt/material-ui/* /material-ui/:splat 301 | |
/zh/material-ui/* /material-ui/:splat 301 | |
/pt/joy-ui/* /joy-ui/:splat 301 | |
/zh/joy-ui/* /joy-ui/:splat 301 | |
/pt/base/* /base/:splat 301 | |
/zh/base/* /base/:splat 301 | |
/pt/system/* /system/:splat 301 | |
/zh/system/* /system/:splat 301 |
We can do this instead to avoid #34820 (comment) and keep /x
redirects independent.
This will allow us to disable translations in MUI X in our own pace.
What do you think?
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.
Makes sense 👍 Would the translation on x still work if this PR gets merged tough?
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.
BTW now that this is committed, make sure to add the redirects in mui/mui-x#6560
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.
Makes sense 👍 Would the translation on x still work if this PR gets merged tough?
I think so - we still have LanguageNegotiation
in our docs.
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.
cc @bharatkashyap should we do something similar for toolpad? Is there support for translations there as well?
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.
No, @mnajdova, there's no need for this in Toolpad since we don't have translations at the moment.
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.
@bharatkashyap
I would suggest removing LanguageNegotiation
in Toolpad docs then:
https://github.com/mui/mui-toolpad/blob/3dccbbe1d4cb992acee77c775c73159beb785b4d/docs/pages/_app.js#L200
Because if you go to https://mui.com/toolpad/getting-started/overview/
and change language - you'll get 404:
https://mui.com/pt/toolpad/getting-started/overview/
Co-authored-by: Andrew Cherniavskii <[email protected]> Signed-off-by: Marija Najdova <[email protected]>
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.
To see if the service workers that developers have previously installed could lead to infinite loops. Can this happen?
- I open http://mui.com/
- The page is served by my service worker (an older version), and I have pt as the selected language.
- The JavaScript logic HTTP redirects me to http://mui.com/pt/
- The new direction we add here HTTP redirects me to http://mui.com/, I'm back to 1. 🔁
How to solve this? One option is to first discontinue language negation of step 3. Release the changes, wait ~a month for the logic to propagate to the cache of people, and do the rest of the changes proposed here.
Another option is at the service worker level. Maybe when it goes from step 4 to 1, the new service worker logic will activate, and break the loop. So maybe we are already good.
Otherwise, it can't think of anything else that could go wrong, it looks good
Co-authored-by: Olivier Tassinari <[email protected]> Signed-off-by: Marija Najdova <[email protected]>
It is a bit risky to test this out by going directly to production. The first option looks safer, I don't mind going with it. @mui/docs-infra do you have a preference? |
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 guess the cleanup of the translation files will be a follow-up PR, right?
@siriwatknp I would personally keep them so it keeps working in dev mode, and until we hear from Albert on whether he's interested in having a focus on growing the Chinese community or not. |
I second what Olivier said. The goal is to just disable the changing of the languages for now. |
For the picker doc on X where the translations are totally out of date and basically useless, can we remove them altogether ? |
@flaviendelangle I think that https://github.com/mui/mui-x/blob/3f2cb51abf2c9456f2a5f7d7bdf1b0b9bc22f969/docs/data/date-pickers/date-picker/date-picker-zh.md is only here because we copied the source from |
Okay, let's keep them and wait for Albert's response. |
@oliviertassinari @siriwatknp I've opened #34844 to address #34820 (review) If we decide to go with that PR, I will schedule a reminder for myself to merge this in a month. |
It would be good to remove the Crowdin logo from the README and docs Backers section. |
It's been a month since #34844 was merged. I will update the PR and merge it so that we can include it in the next release. |
Signed-off-by: Marija Najdova <[email protected]>
Nice, should we do something for MUI X to redirect these pages https://mui.com/pt/x/react-data-grid/? I see that we did some work on this in mui/mui-x#6639. |
I've opened mui/mui-x#7341 to address this |
Closes #33445
Would be great if someone can double check the redirects, are those that I've added enough?