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

[Menu] Deprecate *Props and complete slots, slotProps #44913

Merged
merged 15 commits into from
Feb 19, 2025

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jan 2, 2025

Part of #41281

  • Deprecate MenuListProps and TransitionProps.
  • Added slots.transition and slots.list
  • Added a codemod for migration

@siriwatknp siriwatknp requested a review from DiegoAndai January 2, 2025 07:54
@siriwatknp siriwatknp added the component: menu This is the name of the generic UI component, not the React module! label Jan 2, 2025
@mui-bot
Copy link

mui-bot commented Jan 2, 2025

Netlify deploy preview

TextField: parsed: +0.32% , gzip: +0.26%
@material-ui/core: parsed: +0.09% , gzip: +0.08%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against fc831c9

@siriwatknp siriwatknp marked this pull request as ready for review January 2, 2025 11:47
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 9, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 6, 2025
Comment on lines +206 to +209
const resolvedTransitionProps =
typeof externalForwardedProps.slotProps.transition === 'function'
? externalForwardedProps.slotProps.transition(ownerState)
: externalForwardedProps.slotProps.transition;
Copy link
Member

Choose a reason for hiding this comment

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

Should we use mergeSlotProps here?

Copy link
Member Author

Choose a reason for hiding this comment

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

mergeSlotProps does not concat event handlers, if users provide onEntering it will replace the handleEntering of the Menu. That's why we need to check for typeof function here so that we can:

  onEntering: (...args) => {
    handleEntering(...args); // menu logic
    resolvedTransitionProps?.onEntering?.(...args); // user logic
  },

I don't want to go that far to make mergeSlotProps handle event handlers concatenation because it might effect the performance because the logic will need to loop all slotProps to identify which prop is an event handler (starting with on*).

Comment on lines +229 to +231
typeof slotProps.backdrop === 'function'
? slotProps.backdrop(ownerState)
: slotProps.backdrop,
Copy link
Member

Choose a reason for hiding this comment

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

Should we use mergeSlotProps here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because it will produce a different behavior. The reason to check for typeof function is to use the ownerState of the Menu, not the Popover.

Comment on lines +52 to +55
const transitionProps =
typeof slotProps?.transition === 'function'
? slotProps.transition(ownerState)
: slotProps?.transition;
Copy link
Member

Choose a reason for hiding this comment

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

Should we use mergeSlotProps here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reason as the above comment.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 17, 2025
@siriwatknp siriwatknp added the needs cherry-pick The PR should be cherry-picked to master after merge label Feb 18, 2025
Copy link
Member

@DiegoAndai DiegoAndai 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 the explanations @siriwatknp

@siriwatknp siriwatknp merged commit f273fa5 into mui:master Feb 19, 2025
23 checks passed
siriwatknp added a commit to siriwatknp/material-ui that referenced this pull request Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants