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

[pickers] Add l10n support #4517

Merged
merged 21 commits into from
May 17, 2022
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Apr 15, 2022

Fix #4500

new doc page: https://deploy-preview-4517--material-ui-x.netlify.app/x/react-date-pickers/localization/

Initial solution

To support localization we could define an object with defaultProps to provide needed translations at all components.

I wanted to have something similar to the DataGrid: one file per location. The problem is that we have much more than one component here and it's hard to list for each component which translation key do they need.

In this draft PR What I propose to do is to add a localText prop to every component we export and set the default value with the ThemeProvider

Another option could be to Create a fake component 'MuiPickers' of 'MuiPickersLocale' such that components reads default props of both as follow

const inPropsWithLocale = useThemeProps({ props: inProps, name: 'MuiPickers' });
const props = useThemeProps({ props: inPropsWithLocale, name });

Current Solution

The LocalizationProvider accepts a new prop: localeText that contains all the translations

Components that were setting translation by default have now their props deprecated. If no prop is provided, the local text is obtained from useLocaleText. All keys are available in enUS and contributors can add other translations

About where the defaults are set:

  • the left/right arrows text depends on the component. For calendars it is "prev/next month" for clocks it is "prev/next view" so the default must be set in the parent component
  • the cancel, ok, start, ... were sometimes set in the exported component, sometime in the internal. SInce they do not carry, I put them into the internal component to check deprecated props are not used only once

Works for a follow-up PR

  • Support functions such as the defaultGetClockLabelText
  • Adapt yarn l10n script for the new files

@mui-bot
Copy link

mui-bot commented Apr 15, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 335.6 571.8 390.5 429.64 85.428
Sort 100k rows ms 560.4 1,190.6 1,190.6 927.82 218.923
Select 100k rows ms 187.1 277 226.7 224.92 32.166
Deselect 100k rows ms 116.5 303.9 193.1 205.62 61.081

Generated by 🚫 dangerJS against e95d233

@flaviendelangle
Copy link
Member

@mnajdova @oliviertassinari I would be interested in your opinion about the theme structure here (see @alexfauquette PR description)


Another point : should we drop the prop (rightArrowButtonText for instance) ?
Or at least deprecate them and remove them in a few weeks (we are in alpha but maybe we want the migration to be smooth, as it is not very painful to support both temporarily).

@m4theushw
Copy link
Member

m4theushw commented Apr 20, 2022

The DataGrid has a single entry, the DataGrid, which allows users to change all translations in one place. This doesn't happen with the pickers, you can use DatePicker but also only MobileDatePicker. What is common to all is the LocalizationProvider wrapping them. What about adding localeText to it?

Another point : should we drop the prop (rightArrowButtonText for instance) ?

Yes, I think. In the DataGrid we don't have props to change specific string inside internal components, everything comes from localeText.

@m4theushw m4theushw removed their request for review April 20, 2022 18:51
@alexfauquette
Copy link
Member Author

alexfauquette commented Apr 21, 2022

Following @m4theushw recommendation, I moved the localeText prop to the LocalizationProvider component.

This allows to update the text as follow:

  • import translation from the package
    jsx const theme = createTheme(frFR) <ThemeProvider theme={theme}>
  • set directly localeText to the component
    jsx <LocalizationProvider dateAdapter={AdapterDateFns} localeText={frFR.components.MuiLocalizationProvider.defaultProps.localeText} >

When a prop for text is not undefined it put the following message in the console.

Props for translation are deprecated. See https://mui.com/x/react-date-pickers/localization for more information.
deprecated props observed:
- leftArrowButtonText
- rightArrowButtonText

Also added a page for the localization

@@ -61,7 +61,7 @@ module.exports = {
parserOptions: { tsconfigRootDir: __dirname, project: ['./tsconfig.json'] },
},
{
files: ['docs/src/pages/components/**/*.js', 'docs/src/pages/components/**/*.tsx'],
files: ['docs/data/**/*.js', 'docs/data/**/*.tsx'],
Copy link
Member Author

Choose a reason for hiding this comment

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

Just realized this rule was not applied

@alexfauquette alexfauquette marked this pull request as ready for review April 22, 2022 15:01
@flaviendelangle
Copy link
Member

@alexfauquette the title still contain Draft but the PR is ready for review, can I review it ?

@alexfauquette alexfauquette changed the title [Draft] l10n support l10n support Apr 26, 2022
@alexfauquette
Copy link
Member Author

@flaviendelangle Yes you can review it. Since it is affecting a lot of files, I've redo the commits such as you can review commit by commit

@flaviendelangle
Copy link
Member

Truth is, I would delete the current props instead of deprecating them
I think we are changing to much stuff to handle a real deprecation period.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 26, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@alexfauquette alexfauquette added the on hold There is a blocker, we need to wait label Apr 26, 2022
@alexfauquette
Copy link
Member Author

When removing props, I realized I was breaking the action buttons, because they rely on the definition of cancelText and okText to know if they should appear

Need #4655 to be done in order to distinguish the button content and if it should be rendered

@flaviendelangle
Copy link
Member

Need #4655 to be done in order to distinguish the button content and if it should be rendered

OK, I was thinking that doing #4655 while keeping the old props would be hard
But if I can remove them while doing #4655 then perfect.

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Great work !
That will be so much cleaner with it

const [locale, setLocale] = React.useState<keyof typeof maskMap>('ru');
const [value, setValue] = React.useState<Date | null>(new Date());

const selectLocale = (newLocale: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

We can probably type it correctly

Create a LocalKey type type LocalKey = keyof typeof maskMap and use it both for the state init and here

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a copy past from other pages so I created a dedicated issue #4915


# Pickers - Localization

<p class="description">Pickers allow to support users from different locales, with formatting, RTL, and localized strings.</p>
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
<p class="description">Pickers allow to support users from different locales, with formatting, RTL, and localized strings.</p>
<p class="description">Date and time pickers allow to support users from different locales, with formatting, RTL, and localized strings.</p>

The official naming of the project is "Date and time pickers"
I think we can shortcut with "pickers" when inside a page that already clearly scoped the topic.
But for a title / description we should probably be explicit


Same for the title I think

On the Getting Start page we have "Date / Time pickers" which is not great I think (https://mui.com/x/react-date-pickers/getting-started/)

@samuelsycamore I you have a preference

docs/data/date-pickers/localization/localization.md Outdated Show resolved Hide resolved

Localization can impact pickers components rendering in two distincts ways: The date format, and the components attributes such as `aria-label`.

### Date-engine locale
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan a date-engine but I don't have a better naming

packages/x-date-pickers/src/internals/utils/warning.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 29, 2022
@flaviendelangle flaviendelangle added l10n localization component: pickers This is the name of the generic UI component, not the React module! labels Apr 29, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels May 4, 2022
@github-actions
Copy link

github-actions bot commented May 4, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@alexfauquette alexfauquette self-assigned this May 12, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 17, 2022
@alexfauquette alexfauquette changed the title l10n support [pickers] Add l10n support May 17, 2022
@alexfauquette alexfauquette merged commit aaaadf9 into mui:master May 17, 2022
alexfauquette added a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! l10n localization on hold There is a blocker, we need to wait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Allows community to add new translations
4 participants