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

[base] Do not merge internal ownerState with ownerState from props #36599

Merged
merged 44 commits into from
Apr 10, 2023

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Mar 21, 2023

Closes #36597

This PR handles Task 1 in MUI Core Roadmap and Task 2 in MUI Core Roadmap

Affected components: Joy Menu, Joy Autocomplete, Joy Select, Joy Tooltip, MenuUnstyled, SelectUnstyled

Before

After

@hbjORbj hbjORbj marked this pull request as draft March 21, 2023 18:02
@hbjORbj hbjORbj changed the title [PopperUnstyled] Do not merge internal ownerState with ownerState fro… [PopperUnstyled] Do not merge internal ownerState with ownerState from props Mar 21, 2023
@hbjORbj hbjORbj self-assigned this Mar 21, 2023
@hbjORbj hbjORbj added package: base-ui Specific to @mui/base package: joy-ui Specific to @mui/joy labels Mar 21, 2023
@mui-bot
Copy link

mui-bot commented Mar 21, 2023

Netlify deploy preview

https://deploy-preview-36599--material-ui.netlify.app/

@mui/joy: parsed: +0.04% , gzip: +0.13%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 6c016d2

Comment on lines 149 to 151
slots: {
root: component || 'ul',
},
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test to ensure that the Menu produces ul?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 36231c4

@hbjORbj hbjORbj requested a review from siriwatknp April 4, 2023 16:28
Comment on lines 55 to 76
slots: {
root: {
expectedClassName: classes.root,
},
input: {
testWithComponent: React.forwardRef<HTMLInputElement>((props, ref) => (
<input ref={ref} {...props} data-testid="custom" />
)),
testWithElement: 'input',
expectedClassName: classes.input,
},
listbox: {
testWithComponent: React.forwardRef<HTMLDivElement>((props, ref) => (
<div ref={ref} {...props} data-testid="custom" />
)),
testWithElement: 'div',
expectedClassName: classes.listbox,
},
},
Copy link
Member

Choose a reason for hiding this comment

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

@hbjORbj please add slot tests for other Joy components.

Copy link
Member Author

@hbjORbj hbjORbj Apr 5, 2023

Choose a reason for hiding this comment

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

Gottcha (doing in a separate PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

@hbjORbj
Copy link
Member Author

hbjORbj commented Apr 9, 2023

I just fixed the last error. I think we can merge this. @siriwatknp

@@ -11,6 +11,7 @@
"onClose": "Triggered when focus leaves the menu and the menu should close.",
"open": "Controls whether the menu is displayed.",
"size": "The size of the component (affect other nested list* components because the <code>Menu</code> inherits <code>List</code>). To learn how to add custom sizes to the component, check out <a href=\"/joy-ui/customization/themed-components/#extend-sizes\">Themed components—Extend sizes</a>.",
"slots": "The components used for each slot inside the Popper. Either a string to use a HTML element or a component. See <a href=\"#slots\">Slots API</a> below for more details.",
Copy link
Member

Choose a reason for hiding this comment

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

@hbjORbj Would this be fixed in this PR or the next one?

Screen.Recording.2566-04-10.at.09.50.07.mov

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for fixing it!

@hbjORbj hbjORbj merged commit 41889f4 into mui:master Apr 10, 2023
@hbjORbj hbjORbj changed the title [PopperUnstyled] Do not merge internal ownerState with ownerState from props [base] Do not merge internal ownerState with ownerState from props Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: base-ui Specific to @mui/base package: joy-ui Specific to @mui/joy
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[base] Discard external ownerState in Base components
4 participants