Skip to content

[charts] Add a localization provider #17325

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

Merged
merged 20 commits into from
Apr 23, 2025

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Apr 11, 2025

Close #16963

This introduces the localisation for charts. It will be more useful when we introduce the toolbar since buttons and helpers will need some text.

For info, the data grid put its locales in its apiRef and the pickers in a dedicated context.

I went with the picker approach because we have a similar issue: defining the locale once and using it in multiple components and their subcomponent.

Having a dedicated provider allows targeting it in the MUI theme's defaultProps.

Preview: https://deploy-preview-17325--material-ui-x.netlify.app/x/react-charts/localization/

@alexfauquette alexfauquette added the scope: charts Changes or issues related to the charts product label Apr 11, 2025
Copy link

github-actions bot commented Apr 11, 2025

Thanks for adding a type label to the PR! 👍

Copy link

Please add one type label to categorize the purpose of this PR appropriately:

docs, release, bug, regression, maintenance, dependencies, enhancement or new feature

@alexfauquette alexfauquette added the type: enhancement This is not a bug, nor a new feature label Apr 11, 2025
@mui-bot
Copy link

mui-bot commented Apr 11, 2025

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running pnpm l10n
  • Verify that you have added an export line in src/locales/index.ts for the new locale.
  • Run pnpm docs:api which should add your new translation to the list of exported interfaces.
  • Clean files with pnpm prettier.

Deploy preview: https://deploy-preview-17325--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against 6ec7df8

Copy link

codspeed-hq bot commented Apr 11, 2025

CodSpeed Performance Report

Merging #17325 will not alter performance

Comparing alexfauquette:add-localization (6ec7df8) with master (03408e3)

Summary

✅ 8 untouched benchmarks

Copy link
Member

Choose a reason for hiding this comment

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

Should this be called ChartsLocalizationTableNoSnap? It seems like we're using enUS spelling

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

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

@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 Apr 16, 2025
Copy link

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

@oliviertassinari
Copy link
Member

How does starting to follow the docs structure for l10n in #17475 feel?

@oliviertassinari oliviertassinari added l10n Localization: Work involving translation or adaptation of content for specific languages or regions. type: new feature Introduces a new piece of functionality or capability. and removed type: enhancement This is not a bug, nor a new feature labels Apr 21, 2025
scripts/l10n.ts Outdated
{
key: 'charts',
reportName: '📊 Charts',
constantsRelativePath: 'packages/x-charts/src/constants/defaultLocale.ts',
Copy link
Member

Choose a reason for hiding this comment

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

How does aligning all of those to the pickers feel? Propose in #17475

Suggested change
constantsRelativePath: 'packages/x-charts/src/constants/defaultLocale.ts',
constantsRelativePath: 'packages/x-charts/src/locales/enUS.ts',

*
* - [ChartsLocalizationProvider API](https://mui.com/x/api/charts/charts-localization-provider/)
*/
function ChartsLocalizationProvider(inProps: ChartsLocalizationProviderProps) {
Copy link
Member

@oliviertassinari oliviertassinari Apr 21, 2025

Choose a reason for hiding this comment

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

What's the point of having a ChartsLocalizationProvider? I mean, why do we need this when we have the ThemeProvider?

I guess this is the unstyled version. But then, why don't we have a LocalizationProvider component that is shared between all components?

So overall, it looks like the best path is to:

  • start without ChartsLocalizationProvider no problem solved today, keep it simple
  • generalize the pickers's LocalizationProvider
  • add support for the generalized LocalizationProvider in charts

```jsx
import { createTheme, ThemeProvider } from '@mui/material/styles';
import { BarChart } from '@mui/x-charts/BarChart';
import { frFR } from '@mui/x-charts/locales';
Copy link
Member

Choose a reason for hiding this comment

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

Might be clearer

Suggested change
import { frFR } from '@mui/x-charts/locales';
import { chartsfrFR } from '@mui/x-charts/locales';

Copy link
Member Author

Choose a reason for hiding this comment

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

That would break the logic we use on all packages and their related example.

For now, all packages export locales as follow:

import { xxYY } from '@mui/[packageName]/locales';

And for multiple packages, we encourage them to do

import { xxYY as [packageName]XxYY } from '@mui/[packageName]/locales';

Copy link
Member Author

Choose a reason for hiding this comment

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

While doing the update, I understood your point. Exporting an object that contained only the localeText content.

I propose to export frFR and frFRLocaleText to be more explicit that chartsfrFR that does not says if the content is made for localText prop of the createTheme()

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

I tried the structure proposed in #17475

I moved the ### Built-in locales higher in the order because I felt the need to introduce the information "We have some translation" before explaining how to use them.

I moved the table of available locales completely at the end of the page because it can grow a lot (like the data grid one ) and then cut the reading experience

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 23, 2025
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 23, 2025
@alexfauquette alexfauquette merged commit 0b7a6e8 into mui:master Apr 23, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l10n Localization: Work involving translation or adaptation of content for specific languages or regions. scope: charts Changes or issues related to the charts product type: new feature Introduces a new piece of functionality or capability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Pie chart loading text not localizable
4 participants