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] Add a subset of used @mui/material components to GridSlotsComponent #3490

Merged
merged 15 commits into from
Jan 10, 2022

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Dec 21, 2021

Part of #3066

I've made a subsite of all @mui/material components we use in the grid be available as a slot component. I've checked all the @mui/material we use and these once seemed to make the most sense for a first iteration. I'm adding the rest below. We can discuss and see if any of the others also need to be added in this first iteration (or if I need to revert any of the ones I added).


List of remaining @mui/material components currently used in the grid:

  • CircularProgress
  • TablePagination
  • MenuList
  • MenuItem
  • Paper
  • Grow
  • Popper
  • IconButton
  • ListItemIcon
  • InputBase
  • InputLabel
  • FormControlLabel
  • Box
  • Badge
  • Tooltip
  • NoSsr
  • ClickAwayListener
  • Grow
  • MuiDivider

@DanailH DanailH added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Dec 21, 2021
@DanailH DanailH self-assigned this Dec 21, 2021
@DanailH DanailH changed the title [DataGrid] Add a subset of @mui/material components to GridSlotsComponent [DataGrid] Add a subset of used @mui/material components to GridSlotsComponent Dec 21, 2021
@github-actions
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 Dec 21, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 22, 2021
@flaviendelangle
Copy link
Member

flaviendelangle commented Dec 22, 2021

Could we add a Form input section here ?
To show how to use it to switch to another variant for instance.

@DanailH
Copy link
Member Author

DanailH commented Dec 22, 2021

Could we add a Form input section here ? To show how to use it to switch to another variant for instance.

Sure, but I didn't add all the form elements we use from the core. I skipped InputBase, InputLabel, FormControlLabel. If we are adding a Form input section in the docs maybe its better to also add slots for those components before that?

@flaviendelangle
Copy link
Member

Yes, if you prefer we can first release this feature in a discrete way and then document it fully when ready.

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Should Popper also be added? Adding it seems to solve part of the problem raised in #3381.

It is possible to override Portal props for Column Header tooltips by customizing the theme's default props for Tooltip, but since there is no ability to customize default props for Popper/Portal to propogate the shadow dom container, all of the styled(Popper) components will appear outside and have no styling applied.

@DanailH
Copy link
Member Author

DanailH commented Dec 23, 2021

Should Popper also be added? Adding it seems to solve part of the problem raised in #3381.

It is possible to override Portal props for Column Header tooltips by customizing the theme's default props for Tooltip, but since there is no ability to customize default props for Popper/Portal to propogate the shadow dom container, all of the styled(Popper) components will appear outside and have no styling applied.

I added componentsProps for Popper since we never use it directly. We use it as a base for GridMenu. In addition, I added the Tooltip to slot components.

@DanailH DanailH requested a review from m4theushw December 23, 2021 14:40
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 5, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2022

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 5, 2022
@DanailH DanailH requested a review from m4theushw January 5, 2022 12:51
@DanailH DanailH requested a review from m4theushw January 6, 2022 10:55
@@ -76,7 +78,6 @@ const GridPanel = React.forwardRef<HTMLDivElement, GridPanelProps>((props, ref)
ref={ref}
placement="bottom-start"
className={clsx(className, classes.panel)}
Copy link
Member

Choose a reason for hiding this comment

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

The as prop is missing as well as rootProps.componentsProps.panel.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't add them in the GridPanel directly because the useGridRootProps can be used in components outside of DataGrid. That's why I had to pass it down from the parent. The same applies to the other comment #3490 (comment)

Error: Uncaught Error: MUI: useGridRootProps should only be used inside the DataGrid/DataGridPro component.

Copy link
Member

Choose a reason for hiding this comment

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

We had a discussion about that in #2522.

Currently the GridPanel already depends on another context: GridApiContext. We could also use useGridRootProps but that would be a breaking change. So I'm OK to not do that for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok so if we are postponing this and if this is the only thing left can we proceed with merging this PR?

Comment on lines +25 to 27
as={rootProps.components.BasePopper}
open={columns.length > 0 && preferencePanelState.open}
{...rootProps.componentsProps?.panel}
Copy link
Member

Choose a reason for hiding this comment

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

If the as prop is applied inside GridPanel, then it doesn't need to be applied here again.

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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants