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] Support Checkbox component slot #1528

Merged
merged 13 commits into from
May 7, 2021

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Apr 30, 2021

Closes #281

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Apr 30, 2021

Would we want to create 2 separate component slots, one for checkbox header and another for checkbox cell? Or one?

Currently it is GridCellCheckboxRenderer for cells and GridHeaderCheckbox for checkbox header

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 30, 2021

@ZeeshanTamboli The initial problem I wanted to solve is the customization of the look and feel, not the functional behavior. It was about preparing the unstyled components, so the former option (1 component).

@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review May 2, 2021 10:37
@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 2, 2021

Request for review so that if it looks fine I can move on to cover it up with tests. Only if it would be worth it to have coverage for this?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@ZeeshanTamboli
Copy link
Member Author

Well I thought this was good to take as I saw some upvotes for this at #281 (comment)

@oliviertassinari
Copy link
Member

@ZeeshanTamboli It's still bringing value, my point is that we likely don't want to use the new option in the theming demos.

@EdmundsEcho
Copy link

EdmundsEcho commented May 3, 2021

... but not that I think about it, it feels like this customization should be implemented with the theme overrides.

Quick question on how this approach might work.
In order to customize the icon used for a checkbox the user can use the prop settings for the Mui-Checkbox. e.g., taken from the checkbox demo page:

<Checkbox icon={<FavoriteBorder />} checkedIcon={<Favorite />} name="checkedH" />
  1. Is there a way to control these entries using the style overrides prop?
  2. If a user wanted to customize the prop in the theme, is there a mechanism for selecting/targeting the checkbox for Mui-DataGrid? (i.e., avoid resorting to a default that impact the app globally)

@oliviertassinari
Copy link
Member

@EdmundsEcho if you only want to change the look and feel of the checkboxes in the data grid, and not the whole app, using a components prop once this PR land is probably the most effective. Otherwise, you could nest the theme providers.

@EdmundsEcho
Copy link

EdmundsEcho commented May 3, 2021

@oliviertassinari Thank you for the response.

Has it been confirmed/is there any reason to believe the data grid does not read the default props of the custom theme? I ask because the following entry is not being applied to the data-grid:

...
   props: {
     
      MuiButtonBase: {
        disableRipple: true, 
      },
      
      MuiCheckbox: {
        disableRipple: true,
        checkedIcon: <Check />,
        icon: <Clear />,
      },
    ...
   }
}

Update

I just confirmed that it does update the data grid i.e., it works as expected. Apologies for taking time here.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 3, 2021

@EdmundsEcho If you want to check with components prop for your custom Checkbox I think you can check with this npm package https://pkg.csb.dev/mui-org/material-ui-x/commit/2e9da7f6/@material-ui/data-grid which includes this PR code.

Also I think MuiCheckbox at theme override works because the default checkbox for data-grid is MUI's checkbox.

cc: @oliviertassinari

@ZeeshanTamboli ZeeshanTamboli changed the title [DataGrid]: Support Checkbox component slot [DataGrid] Support Checkbox component slot May 3, 2021
@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels May 3, 2021
…llCheckboxRenderer.tsx

Co-authored-by: Olivier Tassinari <[email protected]>
@oliviertassinari oliviertassinari dismissed their stale review May 4, 2021 12:12

looks good

@oliviertassinari oliviertassinari removed their request for review May 4, 2021 12:13
@EdmundsEcho
Copy link

@ZeeshanTamboli Thank you for the FYI. I greatly appreciate it. I will give the version you pointed me to later this week. In the meantime, I'm somewhat surprised to learn that I can use the overrides key to change the components themselves used for both checked and unchecked states. Said differently, is there a way to use the overrides key to set the component for the unchecked state to <Clear /> from '@material-ui/icons/Clearfor theMuiCheckbox`?

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 4, 2021

@ZeeshanTamboli Thank you for the FYI. I greatly appreciate it. I will give the version you pointed me to later this week. In the meantime, I'm somewhat surprised to learn that I can use the overrides key to change the components themselves used for both checked and unchecked states. Said differently, is there a way to use the overrides key to set the component for the unchecked state to <Clear /> from '@material-ui/icons/Clearfor theMuiCheckbox`?

If you want to customize the checkbox of data grid in any way you want it would be better to use the components prop and use Checkbox slot of data grid once this PR lands.
As for global theme overrides (all checkboxes in global app) since you tried and it works, you can check the Checkbox API for more details on each prop.

is there a way to use the overrides key to set the component for the unchecked state to <Clear />

I think for this you can use icon prop of checkbox. The icon to display when the component is unchecked.
Please create a separate issue for more questions/issues apart from this PR.

@EdmundsEcho
Copy link

EdmundsEcho commented May 4, 2021

Got it. That is my understanding too. I just wanted to make sure I did not miss something; there is no way to set the icon prop using the overrides key, but you can do so using the props key (and once the PR lands, using the data-grid components slot reserved for the checkbox). Thank you!!

@ZeeshanTamboli
Copy link
Member Author

Just commenting to let you'll know this was ready for review.

@oliviertassinari oliviertassinari requested a review from dtassone May 6, 2021 11:31
@dtassone
Copy link
Member

dtassone commented May 6, 2021

Nice and clean 👍

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 6, 2021

Thanks. If we are considering #1552, waiting for it to be merged to make some type changes here as well before I merge this.

@dtassone
Copy link
Member

dtassone commented May 7, 2021

I don't think #1152 is a blocker, this one is ready so merging now. We can rebase #1152 and do the change there.

@dtassone dtassone merged commit db71ba4 into mui:master May 7, 2021
@oliviertassinari
Copy link
Member

I don't think #1152 is a blocker

@dtassone This old close issue doesn't seem to be related, why bring it up? Ah no ok, I think that it's related to #1552.

@ZeeshanTamboli ZeeshanTamboli deleted the issue/281-1438 branch May 7, 2021 11:27
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.

[DataGrid] Extend components API to support custom checkbox
4 participants