-
Notifications
You must be signed in to change notification settings - Fork 833
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
Global theme default props support #1796
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/pgqtrb5q6 |
Test summaryRun 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 |
import { createMuiTheme, ThemeProvider } from '@material-ui/core'; | ||
import { DatePicker, MaterialUiPickersDate } from '@material-ui/pickers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we encourage the following in the mono repository cc @eps1lon. Would it work?
import { DatePicker, MaterialUiPickersDate } from '@material-ui/pickers'; | |
import { DatePickerProps } from '@material-ui/pickers'; | |
DatePickerProps['value'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will
}) => { | ||
const utils = useUtils(); | ||
const classes = useStyles(); | ||
export const DateRangePickerToolbar: React.FC<DateRangePickerToolbarProps> = withDefaultProps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage of makeStyles+withDefaultProps over withStyles? I think that the codebase should be as close as possible to the mono repository. It will be less migration work to do when merging the git repositories. It's also less room for mistake, less confusion for new contributors etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is easier to migrate from the existed codebase and much easier to solve typing issues from injected classes
prop.
I am not tent to rewrite everything to look the same as in the core repository. For example: sure today everybody using makeStyles
instead of withStyles
because it is better for all cases. Just do the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so the issue is with the TypeScript definitions. Can the typing we use in the mono-repository be used to solve the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No really, I am sure that default props
should not be a part of the theme. Because it is not a theme and we have already started discussion of moving them to separate provider, like Configuration
.
So decoupling styles and theme already should help for potential refactorings. And overall I am not seeing any advantages in withStyles
over withDefaultProps
, it is just more friendly to an existing codebase, faster and easier for props type inference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of thoughts:
-
- We have an opportunity to empower a simpler migration once we change the component structure strategy, later on, by using a single approach now.
-
- Assuming we have a ConfigProvider, we could consider moving the default prop logic to this new object, however, it would hurt an issue with theming. it's relatively frequent to change default props that are only style-related:
disableRipple
,color="secondary"
,headlineMapping
, etc.
- Assuming we have a ConfigProvider, we could consider moving the default prop logic to this new object, however, it would hurt an issue with theming. it's relatively frequent to change default props that are only style-related:
-
- If we decouple, we should design the new API to minimize mistakes & inconsistencies. Right now, it's helped by bundling all the features under a single high-order component. For instance: Global theme default props support #1796 (comment).
-
- In [Table] Use makeStyles over withStyles material-ui#15023, we explore a potential migration path to a hook API. I think that the conclusion was that it didn't worth the effort at that point in time.
-
- Looking at how the code look like now and how it would look like in v5, I see a high level of chance that it will go in the opposite direction: default props would move from a hoc to a hook (because it has a smaller footprint), styles will move from a hook to a hoc (because of styled-components).
@dmtrKovalenko Would you agree with the above points?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So decoupling styles and theme already should help for potential refactorings. And overall I am not seeing any advantages in withStyles over withDefaultProps, it is just more friendly to an existing codebase, faster and easier for props type inference
Going back to your questions
- prop type inference is definitely a concern. Higher-order components are hard to type. However, we had to solve this problem in the past for the developers. I don't know this part very well, I believe/hope we did a great job in here, without any significant downside :)
- the advantage of withStyles over withDefaultProps is that it will enable the current API documentation generation stack to output the correct information, without the need for custom work. It will also enable the
getClasses
testing tool to take the component, without custom work. - regarding the performance, I wouldn't expect any difference. I think that the perf win in [Table] Use makeStyles over withStyles material-ui#15023 was related to the removal of a React component, nothing else, which we are introducing and paying now with
withDefaultProps
.
{ name: 'MuiPickersDateRangeDelimiter' }, | ||
styled(Typography)({ | ||
margin: '0 16px', | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing style sheet name, cannot be customized globally (example of what I meant in iii., it requires extra attention)
It seems that the following components have a style sheet global override support but not a default prop global override support:
What should be our strategy with them? |
We have already discussed it, not? |
@dmtrKovalenko Ok, I wasn't aware :) |
Just to clarify: if we will make all our components available for default props – we will not be able to change literally anything without breaking changes. For now, we cannot change class names and public API – and it seems fine. |
This PR closes #1340
Also closes #1806
Description
createMuiThemeBehavior
.