Skip to content
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

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

Merged
merged 6 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions docs/scripts/generateProptypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,30 @@ async function generateProptypes(program: ttp.ts.Program, sourceFile: string) {
if (['children', 'state'].includes(prop.name) && component.name.startsWith('DataGrid')) {
return false;
Comment on lines 68 to 69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
let shouldDocument = true;
let shouldExclude = false;
prop.filenames.forEach((filename) => {
// Don't include props from external dependencies
if (/node_modules/.test(filename)) {
shouldDocument = false;
Comment on lines -73 to -75
Copy link
Member

@oliviertassinari oliviertassinari Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@oliviertassinari oliviertassinari Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙈
Screenshot 2022-12-30 at 11 21 09

Copy link
Member

@oliviertassinari oliviertassinari Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const definedInNodeModule = /node_modules/.test(filename);

if (definedInNodeModule) {
// TODO: xGrid team should consider removing this to generate more correct proptypes as well
if (component.name.includes('Grid')) {
shouldExclude = true;
} else {
const definedInInternalModule = /node_modules\/@mui/.test(filename);
// we want to include props if they are from our internal components
// avoid including inherited `children` and `classes` as they (might) need custom implementation to work
if (
!definedInInternalModule ||
(definedInInternalModule && ['children', 'classes'].includes(prop.name))
) {
shouldExclude = true;
}
}
}
});
return shouldDocument;

// filtering out `prop.filenames.size > 0` removes props from unknown origin
return prop.filenames.size > 0 && !shouldExclude;
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,10 +313,29 @@ DateRangePickerDayRaw.propTypes = {
// | These PropTypes are generated from the TypeScript type definitions |
// | To update them edit the TypeScript types and run "yarn proptypes" |
// ----------------------------------------------------------------------
/**
* A ref for imperative actions.
* It currently only supports `focusVisible()` action.
*/
action: PropTypes.oneOfType([
PropTypes.func,
PropTypes.shape({
current: PropTypes.shape({
focusVisible: PropTypes.func.isRequired,
}),
}),
]),
/**
* If `true`, the ripples are centered.
* They won't start at the cursor interaction position.
* @default false
*/
centerRipple: PropTypes.bool,
/**
* Override or extend the styles applied to the component.
*/
classes: PropTypes.object,
className: PropTypes.string,
/**
* The date to show.
*/
Expand All @@ -336,6 +355,33 @@ DateRangePickerDayRaw.propTypes = {
* @default false
*/
disableMargin: PropTypes.bool,
/**
* If `true`, the ripple effect is disabled.
*
* ⚠️ Without a ripple there is no styling for :focus-visible by default. Be sure
* to highlight the element by applying separate styles with the `.Mui-focusVisible` class.
* @default false
*/
disableRipple: PropTypes.bool,
/**
* If `true`, the touch ripple effect is disabled.
* @default false
*/
disableTouchRipple: PropTypes.bool,
/**
* If `true`, the base button will have a keyboard focus ripple.
* @default false
*/
focusRipple: PropTypes.bool,
/**
* This prop can help identify which element has keyboard focus.
* The class name will be applied when the element gains the focus through keyboard interaction.
* It's a polyfill for the [CSS :focus-visible selector](https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo).
* The rationale for using this feature [is explained here](https://github.com/WICG/focus-visible/blob/HEAD/explainer.md).
* A [polyfill can be used](https://github.com/WICG/focus-visible) to apply a `focus-visible` class to other components
* if needed.
*/
focusVisibleClassName: PropTypes.string,
isAnimating: PropTypes.bool,
/**
* Set to `true` if the `day` is the end of a highlighted date range.
Expand Down Expand Up @@ -365,7 +411,17 @@ DateRangePickerDayRaw.propTypes = {
* Indicates if the day should be visually selected.
*/
isVisuallySelected: PropTypes.bool,
/**
* The component used to render a link when the `href` prop is provided.
* @default 'a'
*/
LinkComponent: PropTypes.elementType,
onDaySelect: PropTypes.func.isRequired,
/**
* Callback fired when the component is focused with a keyboard.
* We trigger a `onFocus` callback too.
*/
onFocusVisible: PropTypes.func,
onMouseEnter: PropTypes.func,
/**
* If `true`, day is outside of month and will be hidden.
Expand All @@ -381,11 +437,41 @@ DateRangePickerDayRaw.propTypes = {
* @default false
*/
showDaysOutsideCurrentMonth: PropTypes.bool,
style: PropTypes.object,
/**
* The system prop that allows defining system overrides as well as additional CSS styles.
*/
sx: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.func, PropTypes.object, PropTypes.bool])),
PropTypes.func,
PropTypes.object,
]),
/**
* @default 0
*/
tabIndex: PropTypes.number,
/**
* If `true`, renders as today date.
* @default false
*/
today: PropTypes.bool,
/**
* Props applied to the `TouchRipple` element.
*/
TouchRippleProps: PropTypes.object,
/**
* A ref that points to the `TouchRipple` element.
*/
touchRippleRef: PropTypes.oneOfType([
PropTypes.func,
PropTypes.shape({
current: PropTypes.shape({
pulsate: PropTypes.func.isRequired,
start: PropTypes.func.isRequired,
stop: PropTypes.func.isRequired,
}),
}),
]),
} as any;

/**
Expand Down
Loading