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

Deprecate TransitionComponentProps #42218

Draft
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

harry-whorlow
Copy link
Contributor

@harry-whorlow harry-whorlow commented May 13, 2024

Part of: #40417, this PR deprecates TransitionComponentProps for the Menu component.

Morning @DiegoAndai, just a draft upload of the menu transitionComponent deprecation, I'll be completing the tests and codemod later this week.

The only question I have about this component is that the menu only has a TranstionProps not a TranstionComponent prop, I presume I just pass the props down like before.... only this time without the TransitionComponent. Either way I'll look into it I just thought I'd ask.

@mui-bot
Copy link

mui-bot commented May 13, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 6377230

@harry-whorlow harry-whorlow changed the title Deprecate TransitionComponent Deprecate TransitionComponentProps May 13, 2024
@DiegoAndai
Copy link
Member

Hey @harry-whorlow! Thanks for picking this up

the menu only has a TranstionProps not a TranstionComponent prop

Then we should

  • Implement slots.transition and slotProps.transition
  • Deprecate TransitionProps in favor of slotProps.transition

@harry-whorlow
Copy link
Contributor Author

awesome, yeah... I just wanted to check before I go off on a tangent.

@harry-whorlow
Copy link
Contributor Author

harry-whorlow commented May 17, 2024

Afternoon @DiegoAndai, sorry to bother you... but, I've been banging my head against a wall over the past few days, so I figured I'd hit you up for some guidance.

You'll see I noted the question in the comment in the code, the two questions exist in Menu.js.

Again sorry for the bother, it's appreciated! 🤟

Comment on lines +185 to +193
// as transitionComponent doesn't exists is this required? or should i be passing it something else,
// I can see its used in the popover component of the popover paper. so perhaps the component isn't needed
const backwardCompatibleSlots = { transition: TransitionComponentProp, ...slots };
const backwardCompatibleSlotProps = { transition: TransitionPropsProp, ...slotProps };

const externalForwardedProps = {
slots: backwardCompatibleSlots,
slotProps: backwardCompatibleSlotProps,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question one

Kind of something like this:
Screenshot 2024-05-17 at 14 51 43

Comment on lines +195 to +204
// am I correct in thinking that this replaces menu as you need to provide it the transitionProps
// or should this wrap the menu component? Because in all other examples there was a pre existing transition component
const [TransitionSlot, transitionProps] = useSlot('transition', {
elementType: MenuRoot,
externalForwardedProps,
ownerState,
});

return (
<MenuRoot
<TransitionSlot
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question two

Or am I incorrect in that assumption and it should be more something like this... (Avatar.js)
Screenshot 2024-05-17 at 14 56 56

@DiegoAndai
Copy link
Member

Hey @harry-whorlow! thanks for working on this. Please see #41281 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants