[DatePicker] Replace withDefaultProps with useThemeProps #24309

m4theushw · 2021-01-08T00:41:04Z

I have followed (at least) the PR section of the contributing guide.

mui-pr-bot · 2021-01-08T00:43:35Z

Generated by 🚫 dangerJS against 72cf4ce

eps1lon · 2021-01-08T05:08:42Z

packages/material-ui-lab/src/DateRangePicker/makeDateRangePicker.tsx

+      AllSharedDateRangePickerProps<TDate> &
+      PublicWrapperProps<TWrapper>,
+  ) {
+    const propsWithDefault = useThemeProps({ props, name });


We should use the shortest name for the one that should be used throughout the component. I propose we use inputProps for the function parameter and then props for the final object.

That way we don't constantly have to change the actual implementation (e.g. by renaming variables).

inputProps is already used in other places and recognized by some internal static analysis tools.

Agree, I think that we should use the existing naming convention:

https://github.com/mui-org/material-ui/blob/a4b5e5537a824bb8b7521ac1146863047e97644f/packages/material-ui/src/Button/Button.js#L324-L325

Disregard #24309 (comment) since there's no reason given. The name should be inputProps.

no reason given

The initial comment was using the consistency argument.

Regarding why @mnajdova used this naming convention, if I had to guess I would assume:

to minimize the diff during the refactoring.

to keep the rendering logic idiomatic (to use "prop").

But best to ask her.

I'll follow #24309 (comment). Does everyone agree? I found more occurrences of inputProps than inProps.

@eps1lon I don't have a preference in the final name we pick (Marija used two different names, so she has an internal conflict about what's better too 😄). I think that we should resolve this question once for all. This is why I think normalizing to a single name is important (and no, not important enough to have a rule in the CI). Once it's the case, developers can follow what the majority does. They can copy.

I think that we should resolve this question once for all. This is why I think normalizing to a single name is important (and no, not important enough to have a rule in the CI)

There's no reason given from your part. You just applied circular logic: "I think we should resolve it, therefore we should change it".

@eps1lon Ok, I can be clearer: "the same solution should be applied to the same problems", this is an axiom. I think we can all agree on it. I think that we should also extend it to "until proven otherwise, problems are identical", which aims to minimize entropy, it helps move things forward as a single consistent "block", to be more effective and efficient, to force confrontation of opposite ideas.

Then comes this specific case. I think that we should make a case for why different names need to be used (why it's a different problem that require a different solution), not the opposite.

I don't care. Just don't break existing code and use inputProps where the name is required, thanks.

I updated this PR to reflect the changes you all requested. In the end I ended up using inProps because inputProps was already being used. I don't think this PR is the best place to discuss these things. If consistency is a big concern let's open a RFC to discuss this and other inconsistencies. It would be nice to enrich the API Design page with variable naming conventions.

oliviertassinari

Great to see this bug taking care of :)

oliviertassinari · 2021-01-08T11:02:16Z

packages/material-ui-lab/src/DateRangePicker/makeDateRangePicker.tsx

+      AllSharedDateRangePickerProps<TDate> &
+      PublicWrapperProps<TWrapper>,
+  ) {
+    const propsWithDefault = useThemeProps({ props, name });


Agree, I think that we should use the existing naming convention:

https://github.com/mui-org/material-ui/blob/a4b5e5537a824bb8b7521ac1146863047e97644f/packages/material-ui/src/Button/Button.js#L324-L325

packages/material-ui-lab/src/DatePicker/DatePicker.test.tsx

packages/material-ui-lab/src/DateRangePicker/DateRangePicker.test.tsx

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

…est.tsx Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

oliviertassinari · 2021-01-12T10:22:43Z

packages/material-ui-lab/src/internal/pickers/Picker/makePickerWithState.tsx

@@ -90,7 +91,7 @@ export function makePickerWithStateAndWrapper<
    );
  }

-  const FinalPickerComponent = withDefaultProps({ name }, withDateAdapterProp(PickerWithState));
+  const FinalPickerComponent = withDateAdapterProp(PickerWithState);


There is a proposal to drop this HOC too in #23670.

oliviertassinari · 2021-01-12T10:23:27Z

packages/material-ui-lab/src/DateRangePicker/makeDateRangePicker.tsx

@@ -1,6 +1,6 @@
 import * as React from 'react';
+import { useThemeProps } from '@material-ui/core/styles';


There is a proposal to expose it as unstable in #24307.

oliviertassinari · 2021-01-12T10:24:13Z

@m4theushw Well done :)

[DatePicker] Replace withDefaultProps with useThemeProps

5f7faa8

Rerun CI

c54d988

github-actions bot mentioned this pull request Jan 8, 2021

Update snapshots for pr/24309 eps1lon/mui-scripts-incubator#1668

Closed

eps1lon reviewed Jan 8, 2021

View reviewed changes

oliviertassinari added component: date picker This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Jan 8, 2021

oliviertassinari reviewed Jan 8, 2021

View reviewed changes

m4theushw and others added 3 commits January 10, 2021 14:01

Update packages/material-ui-lab/src/DatePicker/DatePicker.test.tsx

96e74e8

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Update packages/material-ui-lab/src/DateRangePicker/DateRangePicker.t…

75e5929

…est.tsx Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Rename variables

72cf4ce

oliviertassinari approved these changes Jan 11, 2021

View reviewed changes

oliviertassinari requested a review from eps1lon January 11, 2021 20:25

eps1lon approved these changes Jan 12, 2021

View reviewed changes

oliviertassinari reviewed Jan 12, 2021

View reviewed changes

oliviertassinari merged commit 6b18675 into mui:next Jan 12, 2021

mnajdova mentioned this pull request Jan 14, 2021

[CHANGELOG] Prepare v5.0.0-alpha.23 #24406

Merged

1 task

m4theushw deleted the useThemeProps branch April 15, 2021 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DatePicker] Replace withDefaultProps with useThemeProps #24309

[DatePicker] Replace withDefaultProps with useThemeProps #24309

m4theushw commented Jan 8, 2021

mui-pr-bot commented Jan 8, 2021 •

edited

Loading

eps1lon Jan 8, 2021

oliviertassinari Jan 8, 2021 •

edited

Loading

eps1lon Jan 8, 2021

oliviertassinari Jan 8, 2021 •

edited

Loading

m4theushw Jan 8, 2021

oliviertassinari Jan 9, 2021 •

edited

Loading

eps1lon Jan 10, 2021

oliviertassinari Jan 10, 2021 •

edited

Loading

eps1lon Jan 10, 2021

m4theushw Jan 10, 2021

oliviertassinari left a comment

oliviertassinari Jan 8, 2021 •

edited

Loading

oliviertassinari Jan 12, 2021

oliviertassinari Jan 12, 2021

oliviertassinari commented Jan 12, 2021

		@@ -1,6 +1,6 @@
		import * as React from 'react';
		import { useThemeProps } from '@material-ui/core/styles';

[DatePicker] Replace withDefaultProps with useThemeProps #24309

[DatePicker] Replace withDefaultProps with useThemeProps #24309

Conversation

m4theushw commented Jan 8, 2021

mui-pr-bot commented Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jan 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jan 12, 2021

mui-pr-bot commented Jan 8, 2021 •

edited

Loading

oliviertassinari Jan 8, 2021 •

edited

Loading

oliviertassinari Jan 8, 2021 •

edited

Loading

oliviertassinari Jan 9, 2021 •

edited

Loading

oliviertassinari Jan 10, 2021 •

edited

Loading

oliviertassinari Jan 8, 2021 •

edited

Loading