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

Implement text props name convention #1773

Merged
merged 19 commits into from
May 19, 2020
Merged

Conversation

dmtrKovalenko
Copy link
Member

This PR resolves the first part of #1654 and probably closes #1429

One question as for me. For now, all of our text props are spread through the different components, like PickersModalDialog, ArrowSwitcher, and KeyboardDateInput. It looks like that will be too awful to localize.

Maybe we should add one prop like texts with all the text props combined in 1 object? But it is still possible to pass any prop to the just DatePicker, so support localization only for root components could be a workaround as well

@vercel
Copy link

vercel bot commented May 14, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/1zmba7wng
✅ Preview: https://material-ui-pickers-git-feature-props-localization.mui-org.now.sh

@cypress
Copy link

cypress bot commented May 14, 2020



Test summary

77 0 1 0


Run details

Project material-ui-pickers
Status Passed
Commit 22cf4b1
Started May 19, 2020 2:08 PM
Ended May 19, 2020 2:11 PM
Duration 03:06 💡
OS Linux Debian - 9.11
Browser Chrome 78

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@oliviertassinari
Copy link
Member

oliviertassinari commented May 15, 2020

Regarding #1429, I would propose we close it once we have the counterpart of the localization strings in the mono repository, probably with a working demo in https://material-ui.com/guides/localization/#example. I think that we need to make sure it works, end-to-end. It would also be a great constrain to avoid regressions. What do you think?

Maybe we should add one prop like texts with all the text props combined in 1 object?

Agree, I think that after 5 localization strings for a single component, we should start to wonder. With 10, a texts likrbprop sounds required. Should we call it texts or something else?

There is one aspect I wonder about. How should we handle the strings duplication and fragmentation? Do you have an example of what the change to locale.ruRu look like to have to have the date picker components localized in Russian?

@dmtrKovalenko
Copy link
Member Author

I propose the following strategy:

  1. Merge this PR with only renaming prop
  2. Work for another PR with default props support (Default props support missing #1340)
  3. Work on Localization PR with examples and support (and there combine props into new object)

@oliviertassinari
Copy link
Member

oliviertassinari commented May 18, 2020

@dmtrKovalenko This plan sounds awesome!

const onCloseMock = jest.fn();
const onChangeMock = jest.fn();
const component = mount(
<MobileDatePicker
renderInput={props => <TextField {...props} />}
autoOk
showTodayButton
cancelLabel="stream"
cancelText="stream"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for providing a string in this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is probably not the best way, but I am testing typescript bindings for props such way.
Probably need to add some special kind of typescript definition tests

Copy link
Member

Choose a reason for hiding this comment

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

lib/src/_shared/PickerModalDialog.tsx Outdated Show resolved Hide resolved
lib/src/_shared/PickerModalDialog.tsx Outdated Show resolved Hide resolved
lib/src/_shared/PickerModalDialog.tsx Outdated Show resolved Hide resolved
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@vercel vercel bot temporarily deployed to Preview May 19, 2020 09:58 Inactive
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@vercel vercel bot temporarily deployed to Preview May 19, 2020 09:58 Inactive
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari
Copy link
Member

@dmtrKovalenko We have an ongoing thread on the naming convention for these props in mui/material-ui#21018 (comment).

@dmtrKovalenko dmtrKovalenko merged commit 9c9ab32 into next May 19, 2020
@dmtrKovalenko dmtrKovalenko deleted the feature/props-localization branch May 19, 2020 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add built-in localization support
2 participants