[fields] Fix multi input fields root element props order and types #7225

LukasTy · 2022-12-16T14:19:51Z

Fixes Multi Input Range fields root element
- props to extend StackProps
- spread {...props} at the end to allow for our defined prop value overriding
Update generateProptypes to generate more correct final PropTypes
- skip classes & children from inherited types
Adjust component props removing illogical props

mui-bot · 2022-12-16T14:24:49Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-7225--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	599.7	882	624.8	700.18	112.25
Sort 100k rows	ms	542.3	1,024.8	808.4	810.94	155.147
Select 100k rows	ms	193.2	295.7	218.8	237.22	39.508
Deselect 100k rows	ms	139.1	295.6	162.7	188.84	58.209

Generated by 🚫 dangerJS against 55a3c0b

patches/@mui+monorepo+5.10.16.patch

flaviendelangle

Great job !
I just added a few suggestions of props people can't override

packages/x-date-pickers-pro/src/MultiInputDateTimeRangeField/MultiInputDateTimeRangeField.tsx

flaviendelangle · 2022-12-19T15:15:47Z

packages/x-date-pickers/src/DateField/DateField.tsx

+  /**
+   * Type of the `input` element. It should be [a valid HTML5 input type](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#Form_%3Cinput%3E_types).
+   */
+  type: PropTypes.oneOf([


We can skip this one, we are forcing a value on our side and I don't think people should override it
Or if we want people to override it, we should put it before the spread

I believe that we are changing inputmode and type can easily be changed externally. 🤔
But I agree, that for this kind of component we should not allow it to change. 😉 👍

Oh you are right I mixed both of them sorry 😬

Did we consider the option to copy the solution the main repo uses to fix this problem?

Suggested change

type: PropTypes.oneOf([

type: PropTypes /* @typescript-to-proptypes-ignore */.string,

Nope, we did not look into such solution. But as Flavien mentioned, this change has eventually been reverted in favor of not supporting custom type value, because it does not make sense for these components.

packages/x-date-pickers/src/DateField/DateField.tsx

alexfauquette

This new propTypes strategy sounds to be a good compromise 👍

oliviertassinari · 2022-12-21T22:27:14Z

packages/x-date-pickers/src/DateField/DateField.tsx

+  /**
+   * Type of the `input` element. It should be [a valid HTML5 input type](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#Form_%3Cinput%3E_types).
+   */
+  type: PropTypes.oneOf([


Did we consider the option to copy the solution the main repo uses to fix this problem?

Suggested change

type: PropTypes.oneOf([

type: PropTypes /* @typescript-to-proptypes-ignore */.string,

oliviertassinari · 2022-12-21T22:39:17Z

packages/x-date-pickers-pro/src/SingleInputDateRangeField/SingleInputDateRangeField.tsx

+  /**
+   * This prop helps users to fill forms faster, especially on mobile devices.
+   * The name can be confusing, as it's more like an autofill.
+   * You can learn more about it [following the specification](https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofill).
+   */
+  autoComplete: PropTypes.string,
+  /**
+   * If `true`, the `input` element is focused during the first mount.
+   * @default false
+   */
+  autoFocus: PropTypes.bool,
+  className: PropTypes.string,
+  /**
+   * The color of the component.
+   * It supports both default and custom theme colors, which can be added as shown in the
+   * [palette customization guide](https://mui.com/material-ui/customization/palette/#adding-new-colors).
+   * @default 'primary'
+   */
+  color: PropTypes.oneOf(['error', 'info', 'primary', 'secondary', 'success', 'warning']),


From the DX perspective, I'm not sure that any of the TextField APIs should be duplicated here. I wonder if we shouldn't lean on this approach instead: https://mui.com/material-ui/api/menu/#inheritance. The advantage is that it makes it clear where the props are coming from, and what's the structure of the component. The downside is that people have to learn the pattern once. If they already learned it from MUI Core, they should be good.

Beyond the docs DX question, it would also solve this problem: #7225 (comment).

Suggested change

/**

* This prop helps users to fill forms faster, especially on mobile devices.

* The name can be confusing, as it's more like an autofill.

* You can learn more about it [following the specification](https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofill).

*/

autoComplete: PropTypes.string,

/**

* If `true`, the `input` element is focused during the first mount.

* @default false

*/

autoFocus: PropTypes.bool,

className: PropTypes.string,

/**

* The color of the component.

* It supports both default and custom theme colors, which can be added as shown in the

* [palette customization guide](https://mui.com/material-ui/customization/palette/#adding-new-colors).

* @default 'primary'

*/

color: PropTypes.oneOf(['error', 'info', 'primary', 'secondary', 'success', 'warning']),

I have tried to fix the inheritance automatic docs generation in #7294, I got one step closer, but it wasn't enough to make it work.

From the DX perspective, I'm not sure that any of the TextField APIs should be duplicated here. I wonder if we shouldn't lean on this approach instead: https://mui.com/material-ui/api/menu/#inheritance. The advantage is that it makes it clear where the props are coming from, and what's the structure of the component. The downside is that people have to learn the pattern once. If they already learned it from MUI Core, they should be good.

Really good point. I did not investigate the MUI core solution to this issue deep enough.
In regards to api documentation your point makes total sense and would allow better differentiation between component and inherited props.
But it still leaves some open questions on edge cases:

what if we inherit only some of the component props? I'd imagine it might cause a bit of confusion as to which props are actually valid

what about PropTypes then? They may once again be way out of sync, with what props are actually available on a component. Or we don't care that much about their correctness because a possibly small amount of users are not using TS?

what if we inherit only some of the component props?

@LukasTy The pattern I proposed is not an option in this case. It only works when the component forwards all the props.

what about PropTypes then?

I think that the prop-type should stay broad and links the source, like in this example: https://mui.com/material-ui/api/tree-item/#props TransitionProps.

oliviertassinari · 2022-12-21T22:54:13Z

docs/scripts/generateProptypes.ts

      if (['children', 'state'].includes(prop.name) && component.name.startsWith('DataGrid')) {
        return false;


The changes made me think that we forgot to clean up the list in the main repo: mui/material-ui#35571.

oliviertassinari · 2022-12-21T23:11:21Z

docs/scripts/generateProptypes.ts

-        // Don't include props from external dependencies
-        if (/node_modules/.test(filename)) {
-          shouldDocument = false;


Maybe we should differentiate between peer dependencies and dependencies. For example, MUI X has the TextField as a peer dependency, so MUI X doesn't control which props from the API are supported or not. So the generation could be off. It could tell developer a prop value isn't there when using a version of MUI X that was built with mui/material@v5.6.0 while the developer is on mui/material@v5.7.0.

That is a good point I did not think about. 🤔
So, all in all, MUI (core) preference is declare inheritance and don't focus on having correct/full PropTypes?

don't focus on having correct/full PropTypes?

MUI Core avoids duplicating the prop-types. If the prop-type is fully declared, then when the component receives a wrong prop, the component warns twice: once in the parent and once in the actually inherited component. It assumes it's clearer to not duplicate the prop-type, to warn once and in the lower level. It teaches developers the lower-level component structure.

I think that how the docs display the information is a different topic from how the prop-type handles the case. Soon or later, we will need to generate the API docs from the types, not from the prop-types.

MUI Core avoids duplicating the prop-types. If the prop-type is fully declared, then when the component receives a wrong prop, the component warns twice: once in the parent and once in the actually inherited component. It assumes it's clearer to not duplicate the prop-type, to warn once and in the lower level. It teaches developers the lower-level component structure.

It seems that this problem exists in MUI Core as well.
Did a small test with DateField component in a fresh JS project and my change added one extra warn/error as you mentioned, but there already are 4 others being thrown by the inherited components. 🙈

The suffix 2 is strange on these component names.

Yeah, definitely. From what I recall, we decided to duplicate some of the props because the UX of the inherited prop is not good enough on the API pages. Too many developers where asking the same question: can I use prop xxx? The relevant issue: mui/material-ui#18288 to fix this.

Fix multi input fields root element props order and types

d06afb4

LukasTy added component: pickers This is the name of the generic UI component, not the React module! feature: Keyboard editing Related to the pickers keyboard edition labels Dec 16, 2022

LukasTy requested review from flaviendelangle and alexfauquette December 16, 2022 14:19

LukasTy self-assigned this Dec 16, 2022

LukasTy added 2 commits December 19, 2022 15:17

Update generateProptypes to use only more relevant props

29bed4a

Adjust fields types removing illogical props

98c8ef5

LukasTy commented Dec 19, 2022

View reviewed changes

patches/@mui+monorepo+5.10.16.patch Outdated Show resolved Hide resolved

flaviendelangle reviewed Dec 19, 2022

View reviewed changes

LukasTy added 2 commits December 19, 2022 18:04

Export type via /internals

9fa55bc

Omit error and type props

467a916

alexfauquette approved these changes Dec 20, 2022

View reviewed changes

flaviendelangle approved these changes Dec 20, 2022

View reviewed changes

docs:api

55a3c0b

LukasTy merged commit 82acd33 into mui:next Dec 20, 2022

oliviertassinari reviewed Dec 21, 2022

View reviewed changes

LukasTy deleted the fix-multi-input-field-props-and-types branch December 22, 2022 06:46

LukasTy mentioned this pull request Dec 23, 2022

[pickers] Use grid for modifying the layout #6900

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fields] Fix multi input fields root element props order and types #7225

[fields] Fix multi input fields root element props order and types #7225

LukasTy commented Dec 16, 2022 •

edited

Loading

mui-bot commented Dec 16, 2022 •

edited

Loading

flaviendelangle left a comment

flaviendelangle Dec 19, 2022

LukasTy Dec 19, 2022

flaviendelangle Dec 19, 2022

oliviertassinari Dec 21, 2022 •

edited

Loading

LukasTy Dec 22, 2022

alexfauquette left a comment

oliviertassinari Dec 21, 2022 •

edited

Loading

oliviertassinari Dec 21, 2022 •

edited

Loading

LukasTy Dec 22, 2022

oliviertassinari Dec 22, 2022 •

edited

Loading

oliviertassinari Dec 21, 2022

oliviertassinari Dec 21, 2022 •

edited

Loading

LukasTy Dec 22, 2022

oliviertassinari Dec 22, 2022 •

edited

Loading

LukasTy Dec 30, 2022

oliviertassinari Dec 31, 2022 •

edited

Loading

	type: PropTypes.oneOf([
	type: PropTypes /* @typescript-to-proptypes-ignore */.string,

		if (['children', 'state'].includes(prop.name) && component.name.startsWith('DataGrid')) {
		return false;

[fields] Fix multi input fields root element props order and types #7225

[fields] Fix multi input fields root element props order and types #7225

Conversation

LukasTy commented Dec 16, 2022 • edited Loading

mui-bot commented Dec 16, 2022 • edited Loading

flaviendelangle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette left a comment

Choose a reason for hiding this comment

oliviertassinari Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 31, 2022 • edited Loading

Choose a reason for hiding this comment

LukasTy commented Dec 16, 2022 •

edited

Loading

mui-bot commented Dec 16, 2022 •

edited

Loading

oliviertassinari Dec 21, 2022 •

edited

Loading

oliviertassinari Dec 21, 2022 •

edited

Loading

oliviertassinari Dec 21, 2022 •

edited

Loading

oliviertassinari Dec 22, 2022 •

edited

Loading

oliviertassinari Dec 21, 2022 •

edited

Loading

oliviertassinari Dec 22, 2022 •

edited

Loading

oliviertassinari Dec 31, 2022 •

edited

Loading