Global theme default props support #1796

dmtrKovalenko · 2020-05-20T17:28:52Z

This PR closes #1340
Also closes #1806

Description

Support global props patching through the theme.
Provide typescript typings for overriding default createMuiThemeBehavior.
Properly reexport all the component prop interfaces from the root.

vercel · 2020-05-20T17:28:57Z

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/pgqtrb5q6
✅ Preview: https://material-ui-pickers-git-feature-global-default-props.mui-org.now.sh

cypress · 2020-05-20T17:30:56Z

Test summary

78 • 0 • 1 • 0

Run details


Project	material-ui-pickers
Status	Passed
Commit	`47f0271`
Started	May 25, 2020 1:29 PM
Ended	May 25, 2020 1:30 PM
Duration	01:20 💡
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 · 2020-05-21T15:38:58Z

docs/pages/guides/CssOverrides.example.tsx

 import { createMuiTheme, ThemeProvider } from '@material-ui/core';
+import { DatePicker, MaterialUiPickersDate } from '@material-ui/pickers';


I believe we encourage the following in the mono repository cc @eps1lon. Would it work?

Suggested change

import { DatePicker, MaterialUiPickersDate } from '@material-ui/pickers';

import { DatePickerProps } from '@material-ui/pickers';

DatePickerProps['value']

Yes it will

oliviertassinari · 2020-05-21T15:49:29Z

lib/src/DateRangePicker/DateRangePickerToolbar.tsx

-}) => {
-  const utils = useUtils();
-  const classes = useStyles();
+export const DateRangePickerToolbar: React.FC<DateRangePickerToolbarProps> = withDefaultProps(


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.

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

Ok so the issue is with the TypeScript definitions. Can the typing we use in the mono-repository be used to solve the problem?

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

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.

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?

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.

oliviertassinari · 2020-05-25T14:21:56Z

lib/src/DateRangePicker/DateRangeDelimiter.tsx

+  { name: 'MuiPickersDateRangeDelimiter' },
+  styled(Typography)({
+    margin: '0 16px',
+  })


missing style sheet name, cannot be customized globally (example of what I meant in iii., it requires extra attention)

oliviertassinari · 2020-05-25T14:31:58Z

It seems that the following components have a style sheet global override support but not a default prop global override support:

PureArrowSwitcher
PickerModalDialog
PickerToolbar
ToolbarText
PureDateRangeDay
DateRangePickerInput
DateRangePickerViewDesktop
DateTimePickerTabs
CalendarHeader
Month
MonthSelection
SlideTransition
Year
YearSelection
ClockNumber
StaticWrapper

What should be our strategy with them?

dmtrKovalenko · 2020-05-25T14:38:05Z

We have already discussed it, not?
Make all the component stylable, but provide default props only for exported components to prevent monkey patching internal components.

oliviertassinari · 2020-05-25T14:45:50Z

@dmtrKovalenko Ok, I wasn't aware :)

dmtrKovalenko · 2020-05-25T14:47:21Z

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.

dmtrKovalenko added 22 commits May 11, 2020 12:27

Support empty dates for the DatePicker

bd67a30

Support empty dates for TimePicker

72d2131

Support empty dates for mobile toobars

53db747

Support empty dates in YearSelection

831c4c0

Introduce toolbarPlaceholder prop for customization of empty date text

0e08528

Add tests

b54abef

Merge conflicts

964ae4e

Do not dispatch changeFocusedDay on each mount

d7aa218

Fix crashes when rendering empty date with luxon and moment

7e66833

[TimePicker] Do not throw validation error if value={null}

a5524b4

Update props description for TimePicker validation props

37aec81

Rename text props to match convention for built-in localization

a205056

Merge conflicts

f4882cc

Update using props in examples

3fa0d2f

Merge branch 'next' into feature/global-default-props

1e0d88b

Implement withDefaultProps HO

d200d5d

Fix not working CSS overrides typescript example

6443eca

Add tests for overrides typings

41f2b0c

Implement typings for global overrides

055a9fd

Implement default props support for all Pickers components

f9a80c0

Add default props support for all the components

655f7f5

Fix build errors and properly reexport all props

ca95b11

Merge conflicts

939cfd5

vercel bot deployed to Preview May 20, 2020 17:34 View deployment

oliviertassinari reviewed May 21, 2020

View reviewed changes

dmtrKovalenko added 2 commits May 25, 2020 14:09

Remove duplicated renderInput prop

8709b06

Merge conflicts

d41d4de

vercel bot deployed to Preview May 25, 2020 11:11 View deployment

dmtrKovalenko changed the title ~~Feature/global default props~~ Global theme default props support May 25, 2020

Annulate date-io linked type for documentation

d374865

vercel bot deployed to Preview May 25, 2020 12:19 View deployment

Run typescript tests on CI

47f0271

vercel bot deployed to Preview May 25, 2020 13:26 View deployment

dmtrKovalenko merged commit 6cde0ea into next May 25, 2020

dmtrKovalenko deleted the feature/global-default-props branch May 25, 2020 13:59

oliviertassinari reviewed May 25, 2020

View reviewed changes

oliviertassinari mentioned this pull request Jun 2, 2020

Typescript: material-ui-pickers/typings/overrides does not exist #1794

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global theme default props support #1796

Global theme default props support #1796

dmtrKovalenko commented May 20, 2020 •

edited

vercel bot commented May 20, 2020 •

edited

cypress bot commented May 20, 2020 •

edited

oliviertassinari May 21, 2020

dmtrKovalenko May 22, 2020

oliviertassinari May 21, 2020

dmtrKovalenko May 22, 2020

oliviertassinari May 22, 2020

dmtrKovalenko May 25, 2020

oliviertassinari May 25, 2020 •

edited

oliviertassinari May 25, 2020 •

edited

oliviertassinari May 25, 2020 •

edited

oliviertassinari commented May 25, 2020 •

edited

dmtrKovalenko commented May 25, 2020

oliviertassinari commented May 25, 2020 •

edited

dmtrKovalenko commented May 25, 2020

		import { createMuiTheme, ThemeProvider } from '@material-ui/core';
		import { DatePicker, MaterialUiPickersDate } from '@material-ui/pickers';

-import { DatePicker, MaterialUiPickersDate } from '@material-ui/pickers';
+import { DatePickerProps } from '@material-ui/pickers';
+DatePickerProps['value']

Global theme default props support #1796

Global theme default props support #1796

Conversation

dmtrKovalenko commented May 20, 2020 • edited

Description

vercel bot commented May 20, 2020 • edited

cypress bot commented May 20, 2020 • edited

Test summary

Run details

oliviertassinari May 21, 2020

Choose a reason for hiding this comment

dmtrKovalenko May 22, 2020

Choose a reason for hiding this comment

oliviertassinari May 21, 2020

Choose a reason for hiding this comment

dmtrKovalenko May 22, 2020

Choose a reason for hiding this comment

oliviertassinari May 22, 2020

Choose a reason for hiding this comment

dmtrKovalenko May 25, 2020

Choose a reason for hiding this comment

oliviertassinari May 25, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari May 25, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari May 25, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari commented May 25, 2020 • edited

dmtrKovalenko commented May 25, 2020

oliviertassinari commented May 25, 2020 • edited

dmtrKovalenko commented May 25, 2020

dmtrKovalenko commented May 20, 2020 •

edited

vercel bot commented May 20, 2020 •

edited

cypress bot commented May 20, 2020 •

edited

oliviertassinari May 25, 2020 •

edited

oliviertassinari May 25, 2020 •

edited

oliviertassinari May 25, 2020 •

edited

oliviertassinari commented May 25, 2020 •

edited

oliviertassinari commented May 25, 2020 •

edited