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] Conform components forward ref to root component #15425

Merged
merged 1 commit into from Apr 23, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 20, 2019

Cherry-pick improved ref testing from #15387

@eps1lon eps1lon added the test label Apr 20, 2019
@mui-pr-bot
Copy link

No bundle size changes comparing bdaadb5...cc4e636

Generated by 🚫 dangerJS against cc4e636


if (inheritComponent && instance instanceof window.Element) {
const rootHost = findOutermostIntrinsic(wrapper);
assert.strictEqual(instance, rootHost.instance());
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure to fully understand the implication. Does it prevent the Switch.js pattern to pass the ref test? (that has a wrapping div element)

Copy link
Member Author

Choose a reason for hiding this comment

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

Switch is not a conform component (according to the definition you preferred) as far as I know.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but what are we going to do with the Switch component? I "think" (slightly leaning) that we should break it for the InputBase component. It's probably more intuitive to farward the props to the input element. 80% of the time, users can ignore the wrapping div 🤔. The inputProps has created a lot of confusion. How can we do better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but what are we going to do with the Switch component?

Why is this question important here?

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering out of my lack of knowledge of the describeConformance.js logic.
We move from a test that only checks that the ref is forwarded somewhere to a test that checks where the ref is forwarded. Now, I realize we do the same with the other tests, so 👍.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. Should've compiled the relevant conversations into a proper PR description.

@oliviertassinari oliviertassinari merged commit a74f51a into mui:next Apr 23, 2019
@eps1lon eps1lon deleted the test/root-ref branch April 23, 2019 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants