Make all components generics #2045
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/fhn2n4enz |
|
Test summary
Run details
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 |
There was a problem hiding this comment.
A note for later.
I think that it would be great to explore the potential of only exposing a native date object or a string date in the public API. It seems that using a native date object has the potential to be a great way to remove the need for generics and to have examples that work with any date engines. The impact user-land seems small, people would only need to wrap the date with their library when they need to use it.
lib/src/views/Clock/ClockView.tsx
Outdated
| const { | ||
| allowKeyboardControl, | ||
| type, |
| import { withDefaultProps } from '../../_shared/withDefaultProps'; | ||
| import { useDefaultProps } from '../../_shared/withDefaultProps'; |
There was a problem hiding this comment.
This is a step in the right direction 👍 . In hindsight, I wish I pushed more against withDefaultProps and useStyles in the past.
| @@ -95,7 +96,7 @@ export const useStyles = makeStyles( | |||
| muiComponentConfig | |||
| ); | |||
|
|
|||
| export const Clock: React.FC<ClockProps> = withDefaultProps(muiComponentConfig, (props) => { | |||
| export function Clock<TDate>(props: ClockProps<TDate>) { | |||
There was a problem hiding this comment.
Important question. What would it take to use withStyles here?
There was a problem hiding this comment.
I didn't get the question. What the problem of using makeStyles could be?
There was a problem hiding this comment.
We will need to use the HOC API for integrating the components back into the lab. So we might as well do it now.
There was a problem hiding this comment.
No we cannot use HOCs with pickers sources. Because it breaks generics
There was a problem hiding this comment.
In this case, we need to reconsider the approach, what about we expose a native Date object to remove the need for generics? It's the approach used by https://github.com/adobe/react-spectrum/blob/c700898916bbd076bcc63e49d77c16d80623a8e7/packages/%40react-spectrum/calendar/src/Calendar.tsx (even if they are using date-fns), it seems simpler :)
There was a problem hiding this comment.
Also, I wonder, isn't the Autocomplete using generic and withStyles at the same time?
There was a problem hiding this comment.
Generics lives only on the typescript side – autocomplete written in js.
it is possible to use withStyles. Simply by adding withStyles() as typeof CalendarView where CalendarView is generic. BUT why? I would be happy to make lab infrastructure to work without withStylea
There was a problem hiding this comment.
BUT why?
A couple of reasons that come to mind:
- @mnajdova Is working on revamping how we write the components (at the style and unstyled level). I think that normalizing the approach will make her job easier.
- It bundles a couple of behavior that forces us to implement, which is great.
classesAPI, theme default prop behavior. - It's required for the API classes markdown generation to work correctly.
- It's required for the tests getClasses utility to work correctly.
I would be happy to make lab infrastructure to work without withStylea
I think that we should leave this to 1. and focus on the migration for now. But if you have insight, please raise them to Marija :).
Unfortunately, our internal component's props keep returning
unknownfor date values so we must refactor all the components to be generics. Here is the beginning of refactoring – but with all core changes formakePickerWithState