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

[DataGrid] Refactor: create base Popper #16362

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Jan 27, 2025

Part of the design-system agnostic work.

The base Popper component has a different API than material-ui because material-ui's API is hard to adapt for other design-systems, while the other way around is easy.

@romgrk romgrk added the component: data grid This is the name of the generic UI component, not the React module! label Jan 27, 2025
@romgrk romgrk requested a review from a team January 27, 2025 23:49
@mui-bot
Copy link

mui-bot commented Jan 27, 2025

Deploy preview: https://deploy-preview-16362--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 95745f8

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 28, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025
@@ -43,31 +42,24 @@ const useUtilityClasses = (ownerState: OwnerState) => {
return composeClasses(slots, getDataGridUtilityClass, classes);
};

const GridMenuRoot = styled(Popper, {
const GridMenuRoot = styled(NotRendered<GridSlotProps['basePopper']>, {
Copy link
Member

@oliviertassinari oliviertassinari Jan 31, 2025

Choose a reason for hiding this comment

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

I find this diff strange. What if we were to import https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Popper/BasePopper.tsx and style this one instead.

But even then, I'm unclear about the need. Maybe what confuses me is in the order in which we do this. Shouldn't we first move GridMenuRoot out of the unstyled version? Once we do, we can stick to import and wrap Popper. The fewer React component wrappers, the fastest the grid is.

Unless maybe the whole GridMenu component is a styled API, so maybe move the whole thing out of the unstyled data grid? If I'm using Shadcn UI, would I want to replace it, or add sub slots? I think it's how we determine if it's for the unstyled API or Material UI API.

Copy link
Contributor Author

@romgrk romgrk Jan 31, 2025

Choose a reason for hiding this comment

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

What we need to do here is wrap rootProps.slots.basePopper, but it's not available at initialization. I'm using this NotRendered component just to make the typechecking work. At runtime, as={slots.basePopper} needs to be used.

As for the styled/unstyled changes, I'm still evaluating how to do them. Ideally, we'll have a styling solution that doesn't require tangling styles to components at initialization time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants