mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.07k stars 1.26k forks source link

[data grid] columnVisibilityModel breaks `Show all` and `Hide all` buttons in manage columns overlay #11656

Open bharatAscent opened 8 months ago

bharatAscent commented 8 months ago

Steps to reproduce

Link to live example: (required) https://stackblitz.com/edit/react-3b6mkn?file=Demo.tsx Steps:

  1. Click on any column 3 dots to open the popper
  2. Select manage columns
  3. Try to click on Show all or hide all

Current behavior

It does not show or hide all the columns

Expected behavior

It should allow to show or hide all columns

Context

I am trying to hide all the unnecessary columns in order to perform a checkbox selection with required columns

Your environment

npx @mui/envinfo ``` System: OS: Windows 11 10.0.22631 Binaries: Node: 18.15.0 - C:\Program Files\nodejs\node.EXE npm: 9.5.0 - C:\Program Files\nodejs\npm.CMD pnpm: 8.7.6 - ~\AppData\Local\pnpm\pnpm.EXE Browsers: Chrome: Not Found Edge: Chromium (120.0.2210.121) npmPackages: @emotion/react: ^11.11.1 => 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.18 @mui/core-downloads-tracker: 5.14.12 @mui/icons-material: ^5.14.19 => 5.14.19 @mui/lab: ^5.0.0-alpha.147 => 5.0.0-alpha.147 @mui/material: ^5.14.12 => 5.14.12 @mui/private-theming: 5.15.3 @mui/styled-engine: 5.14.12 @mui/system: 5.14.12 @mui/types: 7.2.12 @mui/utils: 5.15.3 @mui/x-data-grid: ^6.18.2 => 6.18.2 @mui/x-date-pickers: ^6.18.2 => 6.18.2 @mui/x-tree-view: 6.0.0-alpha.1 @types/react: 18.2.41 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 ```

Search keywords: datagrid column hide all

michelengelen commented 8 months ago

It seems as having a value in columnVisibilityModel prevents that. Seems like a bug to me, right @cherniavskii?

michelengelen commented 8 months ago

Even when using the simplest example adding a columnVisibilityModel will do. Unrelated to checkbox selection, server side pagination or sth else.

Demo

cherniavskii commented 8 months ago

This is the expected behavior for a controlled model since onColumnVisibilityModelChange is not passed to the data grid, so the model never changes. If you only want to initialize the model and not control it - pass the model through initialState: https://stackblitz.com/edit/react-3b6mkn-jeyhmj?file=Demo.tsx

michelengelen commented 8 months ago

well, if that is the expected behavior (and that's fine) the UI is misleading, since the buttons are available and not disabled, but do nothing when clicking them.

I would say we should probably look into removing the buttons when a columnVisibilityModel is present.

Thoughts @cherniavskii and @joserodolfofreitas ?

joserodolfofreitas commented 8 months ago

I wouldn't remove the buttons in this case, because when the user controls the model, it's expected that only the user updates the model to the DataGrid (so the user needs to intercept show– or hide all and other interactions with the onColumnVisibilityModelChange).

But perhaps we could trigger a warning message if we notice the user sets a controlled model but doesn't set an "interception" function to the onModelChange; that way, we may avoid this unintended behavior.

michelengelen commented 8 months ago

I wouldn't remove the buttons in this case, because when the user controls the model, it's expected that only the user updates the model to the DataGrid (so the user needs to intercept show– or hide all and other interactions with the onColumnVisibilityModelChange).

But perhaps we could trigger a warning message if we notice the user sets a controlled model but doesn't set an "interception" function to the onModelChange; that way, we may avoid this unintended behavior.

That could help. I wasn't aware of that limitation. Maybe we could at least add a comment (infobox) to the related section in the docs?

cherniavskii commented 8 months ago

We can add a warning to each controlled model section, similar to this: https://react.dev/reference/react-dom/components/input#controlling-an-input-with-a-state-variable

joserodolfofreitas commented 8 months ago

An info box at every "controlled" section sounds good. It'd also be good to rephrase the part that says "You can use the onModelChange" to "You must." example from docs:

You can use the onFilterModelChange prop to listen to changes to the filters and update the prop accordingly.

You must use the onFilterModelChange prop to listen to changes to the filters and update the prop accordingly.

We have one for the Date pickers already, although it doesn't mention this particular potential confusion:

image
chiaberry commented 7 months ago

I wouldn't remove the buttons in this case, because when the user controls the model, it's expected that only the user updates the model to the DataGrid (so the user needs to intercept show– or hide all and other interactions with the onColumnVisibilityModelChange).

But perhaps we could trigger a warning message if we notice the user sets a controlled model but doesn't set an "interception" function to the onModelChange; that way, we may avoid this unintended behavior.

How can I intercept show- or hide-- interactions? Currently when I toggle "show all", the column model is sending an object with only one column to the onColumnVisibilityModelChange function. Hide All is sending an object with all columns and false, so that is working. If I could intercept the show all action, and set all columns visibility as true, that would fix this problem.

I tried using the details variable, but it just comes back as {reason: undefined}

function(model: GridColumnVisibilityModel, details: GridCallbackDetails) => void

michelengelen commented 7 months ago

@chiaberry Here is an example: Controlled column visibility model

chiaberry commented 7 months ago

@michelengelen Thanks for that link, i have a similar set up in my project.

I was curious if theres a way to specifically know when the user has changed the model using the show all or hide all interactions. That's where I am seeing unexpected behavior with the object being returned to the onColumnVisibilityModelChange function.

michelengelen commented 7 months ago

We don't have a way to do that in the onColumnVisibilityModelChange prop. Sry! 🙇🏼

nicoprten commented 7 months ago

Anyone know how can I difference the event when is called from 'Show all' or 'Hide all' in onColumnVisibilityModelChange?

michelengelen commented 6 months ago

No, there currently is no way to determine where that change came from. We could however add an optional reason (values could be show/hide-all-from-columns-management-panel) parameter to setColumnVisibilityModel when the button is clicked.

setColumnVisibilityModel: (model: GridColumnVisibilityModel, reason?: string) => void;

This would also give the user a bit more freedom on passing in their custom reasons.

WDYT @mui/xgrid ?