Skip to content
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

Merged
merged 11 commits into from Dec 1, 2022
Merged

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Oct 19, 2022

Closes #33445

Would be great if someone can double check the redirects, are those that I've added enough?

@mnajdova mnajdova added docs Improvements or additions to the documentation i18n internationalization labels Oct 19, 2022
@mui-bot
Copy link

mui-bot commented Oct 19, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-34820--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against 17d66c3

@mnajdova mnajdova marked this pull request as ready for review October 19, 2022 08:16
@mnajdova mnajdova requested review from mbrookes and a team October 19, 2022 08:16
@@ -39,39 +32,6 @@ const clientSideEmotionCache = createEmotionCache();
// Set the locales that the documentation automatically redirects to.
acceptLanguage.languages(LANGUAGES);

function LanguageNegotiation() {
Copy link
Member

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

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/pages/_app.js Outdated Show resolved Hide resolved
@@ -4,10 +4,10 @@ const CODE_VARIANTS = {
};

// Valid languages to server-side render in production
const LANGUAGES = ['en', 'zh', 'pt'];
const LANGUAGES = ['en'];
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

Comment on lines 392 to 393
/pt/* /:splat 301
/zh/* /:splat 301
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/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?

Copy link
Member Author

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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 <andrew.cherniavskii@gmail.com>
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
Copy link
Member

@oliviertassinari oliviertassinari left a 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?

  1. I open http://mui.com/
  2. The page is served by my service worker (an older version), and I have pt as the selected language.
  3. The JavaScript logic HTTP redirects me to http://mui.com/pt/
  4. 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

docs/public/_redirects Outdated Show resolved Hide resolved
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
@mnajdova
Copy link
Member Author

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.

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?

Copy link
Member

@siriwatknp siriwatknp left a 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?

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 20, 2022

@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.

@mnajdova
Copy link
Member Author

I guess the cleanup of the translation files will be a follow-up PR, right?

I second what Olivier said. The goal is to just disable the changing of the languages for now.

@flaviendelangle
Copy link
Member

flaviendelangle commented Oct 20, 2022

@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 community in the Chinese market or not.

For the picker doc on X where the translations are totally out of date and basically useless, can we remove them altogether ?
Or at least reset them to avoid misleading content.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 20, 2022

X where the translations are totally out of date and basically

@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 mui/material-ui, we are not synching mui/mui-x with Crowdin so 👍 to remove.

@siriwatknp
Copy link
Member

I guess the cleanup of the translation files will be a follow-up PR, right?

I second what Olivier said. The goal is to just disable the changing of the languages for now.

Okay, let's keep them and wait for Albert's response.

@mnajdova
Copy link
Member Author

@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.

@mbrookes
Copy link
Member

It would be good to remove the Crowdin logo from the README and docs Backers section.

@mnajdova mnajdova added the on hold There is a blocker, we need to wait label Oct 25, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 25, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 26, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 7, 2022
@mnajdova
Copy link
Member Author

mnajdova commented Dec 1, 2022

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 <mnajdova@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 1, 2022
@mnajdova mnajdova merged commit 2abad79 into mui:master Dec 1, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 1, 2022

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.

feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
@cherniavskii
Copy link
Member

Nice, should we do something for MUI X to redirect these pages 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation i18n internationalization on hold There is a blocker, we need to wait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Disable the translations
9 participants