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

[RFC] Replace MaterialUiPickersDate type with DateType #1962

Closed
oliviertassinari opened this issue Jul 6, 2020 · 3 comments · Fixed by #1966
Closed

[RFC] Replace MaterialUiPickersDate type with DateType #1962

oliviertassinari opened this issue Jul 6, 2020 · 3 comments · Fixed by #1966

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 6, 2020

Looking at the codebase, I believe we could gain in clarity by

  • re-exporting DateType from @date-io/type so we can do import { DateType } from '@material-ui/pickers'
  • replace MaterialUiPickersDate with DateType | null everywhere in the codebase

This discussion first started in #1955 (comment).

Also, I have noticed a couple of places where | null seems redundant, for instance:

https://github.com/mui-org/material-ui-pickers/blob/8f895d5e438dcc8f7f04837763979a3009d777dc/lib/src/views/Calendar/useCalendarState.tsx#L11

For reference:

https://github.com/mui-org/material-ui-pickers/blob/8f895d5e438dcc8f7f04837763979a3009d777dc/lib/src/typings/date.ts#L1-L3

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 6, 2020

Also, it's worth noting that in the Autocomplete, there is a prop that prevents the value to be null (disableClearable). I believe we could imagine something similar in the future for the date picker:

https://github.com/mui-org/material-ui/blob/877c33b9660804a3f319c8f903a4f93cd0a402a5/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.spec.ts#L110-L116

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Jul 6, 2020

It looks like we must get rid of DateType at all. Because it is not working as it should anymore.

Ts 3.9 is not inferring as dynamic type. The reason of #1861

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 6, 2020

Interesting, it sounds like there is work to be done here :). A couple of more thoughts:

  1. including | null inside the type feels too ambitious. I think that it should be explicit each time.
  2. including MaterialUi as a prefix might not be the best move. I would be eager to use something less specific. It would also be a first in the public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants