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

[test] Test all Base components with describeConformanceUnstyled #34825

Merged

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Oct 19, 2022

Unstyled Modal and Popper weren't using all relevant describeConformanceUnstyled's tests and TextareaAutosize was still using Material UI's describeConformance to test basic behavior. This PR fixes these issues.

An issue with the Unstyled Modal was found along the way. It incorrectly prioritized slots.root over component. Fixing this is a breaking change in Base, however, it should not affect most developers, as setting both component and slots.root makes no sense.
This change doesn't affect the Material UI's Modal as it has its own logic for choosing between slots.root and component.

This PR also improves how describeConformanceUnstyled works with portaled components and improves error display in the ownerStatePropagation test.

Breaking change
Prior to this change, if both component and slots.root were defined, slots.root would be used. After this PR, component will be chosen in this case.
There are no use cases for having both component and slots.root defined at the same time. Remove one of them to reduce confusion.

@michaldudak michaldudak added test breaking change component: modal This is the name of the generic UI component, not the React module! component: Popper The React component. See <Popup> for the latest version. component: TextareaAutosize The React component. package: base-ui Specific to @mui/base labels Oct 19, 2022
@mui-bot
Copy link

mui-bot commented Oct 19, 2022

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

Details of bundle changes

Generated by 🚫 dangerJS against 940e91a

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 19, 2022
@michaldudak michaldudak force-pushed the describeConformanceUnstyled-in-base branch from 5a4e341 to 8564736 Compare October 19, 2022 10:00
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 19, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@michaldudak michaldudak merged commit 8a584fd into mui:master Oct 20, 2022
@michaldudak michaldudak deleted the describeConformanceUnstyled-in-base branch October 20, 2022 10:58
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
@wallace-sf
Copy link

The project breaks on line 238 changes. If you pass a Root to ModalUnstyled, the function getHasTransition can't hasOwnProperty from children props.

image

feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
@michaldudak
Copy link
Member Author

@wallace-sf Could you please open a new issue and create a reproduction on codesandbox?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: modal This is the name of the generic UI component, not the React module! component: Popper The React component. See <Popup> for the latest version. component: TextareaAutosize The React component. package: base-ui Specific to @mui/base test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants