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

[Popper][base] Drop component prop #37060

Closed
wants to merge 12 commits into from

Conversation

nicolas-ot
Copy link
Contributor

BREAKING CHANGE

part of #36679
It will be covered by codemod, see #36831

For Base UI components, the component prop value should be moved to slots.root:

<Popper
-  component="span"
+  slots={{ root: "span" }}
 />

If using TypeScript, you should add the custom component type as a generic on the TabsList component.

-<Popper
+<Popper<typeof CustomComponent>
   slots={{ root: CustomComponent }}
   customProp
 />

@mui-bot
Copy link

mui-bot commented Apr 27, 2023

Netlify deploy preview

Bundle size report

Bundle size will be reported once CircleCI build #515804 finishes.

Generated by 🚫 dangerJS against 68c3f82

@zannager zannager added component: Popper The React component. See <Popup> for the latest version. package: base-ui Specific to @mui/base labels Apr 27, 2023
@zannager zannager requested a review from mnajdova April 27, 2023 13:33
@@ -233,7 +231,7 @@ const PopperTooltip = React.forwardRef(function PopperTooltip<
return (
<Root {...rootProps}>{typeof children === 'function' ? children(childProps) : children}</Root>
);
}) as OverridableComponent<PopperTooltipTypeMap>;
}) as PolymorphicComponent<PopperTooltipTypeMap>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaldudak this change seems to make yarn proptypes change the proptypes of Menu.tsx and Select.tsx (both use Popper as default Root component) like in the picture. Any idea what might causes this?
image

Copy link
Member

Choose a reason for hiding this comment

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

You can add /* @typescript-to-proptypes-ignore */ before running yarn proptypes to prevent that change

  container: PropTypes /* @typescript-to-proptypes-ignore */.oneOfType([
    HTMLElementType,
    PropTypes.func,
  ]),

Copy link
Contributor Author

@nicolas-ot nicolas-ot Apr 27, 2023

Choose a reason for hiding this comment

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

Thank you!

ps: would be cool if you can add this PR to #36679

Copy link
Member

Choose a reason for hiding this comment

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

In fact, ignore my comment above, that's not how we should handle it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have you found another way to deal with the problem?

@nicolas-ot nicolas-ot marked this pull request as draft April 27, 2023 14:33
@nicolas-ot nicolas-ot marked this pull request as ready for review April 27, 2023 18:53
@nicolas-ot nicolas-ot marked this pull request as draft April 27, 2023 19:05
@nicolas-ot
Copy link
Contributor Author

one problem left unsolved, running yarn docs:typescript:formatted changes Button.proptypes like the image below. Adding /* @typescript-to-proptypes-ignore */ doesn't seem to solve the issue. Will try to fix in the following days.

image

Comment on lines 126 to 128
> = PolymorphicProps<PopperTypeMap<{}, RootComponentType>, RootComponentType> & {
component?: RootComponentType;
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> = PolymorphicProps<PopperTypeMap<{}, RootComponentType>, RootComponentType> & {
component?: RootComponentType;
};
> = PolymorphicProps<PopperTypeMap<{}, RootComponentType>, RootComponentType>;

Copy link
Contributor Author

@nicolas-ot nicolas-ot Apr 28, 2023

Choose a reason for hiding this comment

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

this Props also describe Popper from mui-material. Since we don't remove component prop from that Popper, this component props can't be removed here. Typescript will give error on the Popper.tsx (packages/mui-material/src/Popper/Popper.tsx)

...or is that not a problem?

Comment on lines 147 to 149
> = PolymorphicProps<PopperTooltipTypeMap<{}, RootComponentType>, RootComponentType> & {
component?: RootComponentType;
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> = PolymorphicProps<PopperTooltipTypeMap<{}, RootComponentType>, RootComponentType> & {
component?: RootComponentType;
};
> = PolymorphicProps<PopperTooltipTypeMap<{}, RootComponentType>, RootComponentType>;

@@ -237,7 +239,9 @@ describe('useSlot', () => {
anchorEl: () => document.createElement('div'),
},
internalForwardedProps: {
component: ItemListbox,
slots: {
root: ItemListbox,
Copy link
Member

Choose a reason for hiding this comment

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

why are you doing this change?

Copy link
Contributor Author

@nicolas-ot nicolas-ot Apr 28, 2023

Choose a reason for hiding this comment

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

that is a test that use base Popper as describeb here elementType: Popper as unknown as 'ul',. I believe internalForwardedProps is a prop that is forwarded to the Popper, hence the change.

@hbjORbj
Copy link
Member

hbjORbj commented May 1, 2023

Thanks a lot for your contribution. @nicolas-ot We need to support dropping component prop throught all Base UI components in today's release, and as you know, Popper is the only remaining component left. Because Base UI Popper is used in some of Joy UI and Material components, we need to do a little more work for it.. Because of the time constraint we have, I will close this PR. Again, thanks for your hard work.

@hbjORbj hbjORbj closed this May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popper The React component. See <Popup> for the latest version. package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants