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.

Sign up for GitHub

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] Add code snippet for localization docs in the data grid #1024

Merged
merged 4 commits into from
Feb 11, 2021
Merged

[docs] Add code snippet for localization docs in the data grid #1024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a59f223
Add code snippet for localization docs in the data grid
DanailH Feb 10, 2021
2353c64
Exlude numeric operators from translations
DanailH Feb 11, 2021
02fc39b
Update docs/src/pages/components/data-grid/localization/localization.md
oliviertassinari Feb 11, 2021
d454459
blank line
oliviertassinari Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions docs/src/pages/components/data-grid/localization/localization.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,30 @@ In the following example, the labels of the density selector are customized.
The default locale of Material-UI is English (United States).
You can find all the locales supported in [the source](https://github.com/mui-org/material-ui-x/blob/HEAD/packages/grid/_modules_/grid/locales) in the GitHub repository.

You can use the theme to configure the locale text:

```jsx
import { createMuiTheme, ThemeProvider } from '@material-ui/core/styles';
import { DataGrid, bgBG } from '@material-ui/data-grid';

const theme = createMuiTheme(
{
palette: {
primary: { main: '#1976d2' },
},
},
bgBG,
);

<ThemeProvider theme={theme}>
<DataGrid />
</ThemeProvider>;
```

oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
Note that `createMuiTheme` accepts any number of arguments.
If you are already using the translations of the core components, you can add `bgBG` as a new argument.
The same import works with `XGrid` as it's an extension of `DataGrid`.
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved

### Supported locales

| Locale | BCP 47 language tag | Import name |
Expand Down
7 changes: 4 additions & 3 deletions packages/grid/_modules_/grid/components/panel/filterPanel/FilterForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,10 @@ export function FilterForm(props: FilterFormProps) {
>
{currentColumn?.filterOperators?.map((operator) => (
<option key={operator.value} value={operator.value}>
{apiRef!.current.getLocaleText(
`filterOperator${capitalize(operator.value)}` as TranslationKeys,
)}
{operator.label ||
apiRef!.current.getLocaleText(
`filterOperator${capitalize(operator.value)}` as TranslationKeys,
Copy link
Member

Choose a reason for hiding this comment

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

that is weird!
Usually, you want to translate labels not value. This bit needs a learning curve, you need to know that the key is prefixed and uppercased... Why not simply translate the label and leave the value alone? Also should we need to throw if a translation key is missing? Imagine I don't want any translations, just english? I shouldn't have to pass local text, but with this approach it would throw

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is much of a learning curve, the translation keys are easily accessible and can be checked. The reason why I removed the label initially is that it isn't really needed, the key can be build from the value without the need of having another prop.

For the second concern - if you want to use English you can, there is no need to pass in the localText prop. We only throw if we are using a nonexistent translation key internally in the grid (the throw is for us to see if something broke).

Copy link
Member

Choose a reason for hiding this comment

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

Well if I create a new operator with value 'between' and I want to translate it, I would have to pass the key filterOperatorBetween right?

Copy link
Member

Choose a reason for hiding this comment

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

Or I would have to pass the translation directly within the label. Then I would have to manage localization in 2 places, in the prop local-text and in the operators which to me, it does not seem convenient.
If you translate labels, then I would pass my key in label and know what to translate in localtext.

Copy link
Member

@oliviertassinari oliviertassinari Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if I create a new operator with value 'between' and I want to translate it

@dtassone I think that we are back to #1024 (comment). I think that we should encourage developers to use the label prop and translate it when needed. localeText is about translating the built-in components. It shouldn't be encouraged for translating customizations.

const filterOperators = [{
  label: 'Between', // to translate
  value: 'between',
}];

<DataGrid columns={[{ id: 'foo', filterOperators }]}

I would have to manage localization in 2 places

Exactly, this improves the DX, the fewer indirections, the better. Developers should already have a localization solution for the rest of their app, i18next, react-intl, else. I think that we should make it easy to use it, not having them to guess what properties to use for dynamically created one in localeText.

Copy link
Member

@dtassone dtassone Feb 11, 2021 •
edited by oliviertassinari
Loading

Choose a reason for hiding this comment

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

I would pass a key in label and override the translation with the local text...
Otherwise I would have to

const filterOperators = [{
  label: i18n.translate('Between'),
  value: 'between',
}];

<DataGrid
  columns={[{ id: 'foo', filterOperators }]}
  localtext={{filterOperatorContains: i18n.translate('has')...}}
/>

I would have expected

const filterOperators = [{
  label: 'OperatorLabelBetween', // that does not prevent me from doing i18n.translate('Between'),
  value: 'between',
}];

<DataGrid
  columns={[{ id: 'foo', filterOperators }]}
  localtext={{OperatorLabelBetween: i18n.translate('Between'), OperatorLabelContains: i18n.translate('has')...}}
/>

Copy link
Member

@oliviertassinari oliviertassinari Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If I was working on a product, I would personally do 1. over 2. without any hesitation. But I can imagine it can depend on the developers. @DanailH do you have a preference?

)}
</option>
))}
</Select>
Expand Down
1 change: 1 addition & 0 deletions packages/grid/_modules_/grid/models/filterOperator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { FilterItem } from './filterItem';
import { CellParams } from './params/cellParams';

export interface FilterOperator {
label?: string;
value: string;
getApplyFilterFn: (
filterItem: FilterItem,
Expand Down