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

Merged
merged 4 commits into from
Feb 11, 2021

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Feb 10, 2021

Fixes #777

The missing docs as mentioned in the last comment.

@DanailH DanailH added docs Improvements or additions to the documentation component: data grid This is the name of the generic UI component, not the React module! labels Feb 10, 2021
@DanailH DanailH self-assigned this Feb 10, 2021
@oliviertassinari oliviertassinari changed the title Add code snippet for localization docs in the data grid [docs] Add code snippet for localization docs in the data grid Feb 10, 2021
@DanailH DanailH requested a review from mbrookes February 10, 2021 13:15
@dtassone
Copy link
Member

You have not translated the numeric operators
image

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2021

@dtassone Please include the URL you have used, it makes it easier to follow. I could reproduce on https://master--material-ui-x.netlify.app/components/data-grid/filtering/. It sounds like something is wrong with the CI, master should be failing or something is wrong with the demos.

@dtassone
Copy link
Member

You have to click the numeric filters
https://deploy-preview-1024--material-ui-x.netlify.app/components/data-grid/filtering/

@DanailH
Copy link
Member Author

DanailH commented Feb 11, 2021

Ok saw the issue. I can exclude the numeric operators from the translations since they are symbols.

@DanailH DanailH requested a review from dtassone February 11, 2021 11:50
@dtassone
Copy link
Member

What if I customise numeric operators with a new one that needs translation?

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2021

What if I customize numeric operators with a new one that needs translation?

@dtassone If you customize it, you can translate it at the same time? I don't follow

@dtassone
Copy link
Member

What if I customize numeric operators with a new one that needs translation?

@dtassone If you customize it, you can translate it at the same time? I don't follow

How?

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2021

@dtassone
Copy link
Member

What if I customize numeric operators with a new one that needs translation?

@dtassone If you customize it, you can translate it at the same time? I don't follow

How?

if numeric operators are not translated then you would have to translate the value, which then would be different for each country.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2021

Sorry, I don't understand what we are talking about. Maybe we can take a step back and start with the user-story. What problem are we trying to solve? So far, there are only one problem I can wrap my head around:

  1. >=, <, etc. needs to be translated because it renders differently in another locale. Does it? Do we have an example of a locale that requires it? For instance, I had a look at Arabic that has different mathematical notations, operators like this seem identical.

@dtassone
Copy link
Member

@DanailH
Copy link
Member Author

DanailH commented Feb 11, 2021

How?

@dtassone The same way this demo is built? https://deploy-preview-1024--material-ui-x.netlify.app/components/data-grid/filtering/#CustomRatingOperator.tsx

Label has been removed

You can check the code. I reintroduced the label as optional. If the label is present them IT will be shown, not the translation.

)}
{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

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

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

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?

@DanailH DanailH merged commit af0ebbb into mui:master Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Support ready-to-use locales
3 participants