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] How to get the visible columns? #1507

Closed
marcellemcm opened this issue Apr 27, 2021 · 11 comments
Closed

[DataGrid] How to get the visible columns? #1507

marcellemcm opened this issue Apr 27, 2021 · 11 comments
Labels
component: data grid This is the name of the generic UI component, not the React module!

Comments

@marcellemcm
Copy link

marcellemcm commented Apr 27, 2021

  • [X ] I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

I am using Data Grid and I have a requirement to export to csv, xls and txt, only the data that is visible in the table.

When clicking on the rendered Column menu component by clicking on the 3-point "kebab" icon in the column headers and then show columns/Hide.

Captura de Tela 2021-04-27 às 19 17 32

In the documentation I didn't find any props to show the column parameters and get the columns visible when this event was triggered .

I know that there is an ApiRef.Column on the X-Grid. But how to know this information in the DataGrid?

@marcellemcm marcellemcm added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 27, 2021
@marcellemcm marcellemcm changed the title [DataGrid] - Get the visible columns after clicking manage columns in the column menu component [DataGrid] Get the visible columns after clicking manage columns in the column menu component Apr 27, 2021
@oliviertassinari
Copy link
Member

@marcellemcm Perfect, this is a continuation of #1435. @dtassone This time, an export flag on the column definition won't cut it. We have a dynamic use case

@dtassone
Copy link
Member

Well I believe @marcellemcm is expecting a handler to get the new visible columns when one gets hidden?
is that correct? Something like onColumnsVisibilityChange or onColumnsUpdated?

FYI, we have an event GRID_COLUMNS_UPDATED that is published when columns change. Maybe you could subscribe to this event as a workaround.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 28, 2021

@dtassone Do you mean, have a callback to know what are the visible rows (to store them in a React state) so they can then be provided as an argument to the export feature?

@dtassone
Copy link
Member

Yes to respect our pattern, we should have those props so users can hook on to those events easily and do something...

FYI The visible columns are already in the grid state. And the export feature already uses the visible columns.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 28, 2021

@dtassone The more we move forward, the more I wonder if the one callback per state pattern is scaling. It feels like there is an endless need for adding new props. But maybe I'm wrong, and it's only because we don't yet have a high coverage.

In any case, I think that we should explore all our options. For instance, we could imagine having a state callback selector, allowing developers to cherrypick, sub parts of the state and replicate them from the data grid (being the source of truth in an uncontrolled mode) to a parent state. Isn't what we had onStateChange for?

The alpha stage is perfect for this type of exploration, it's now that we can freeze what pattern works and which one doesn't.

@dtassone
Copy link
Member

The idea is to make the component easy to use and approachable to users so they get where they want in a rapid manner.
A grid is a complex component so it will end up with a large number of optional props.
While the props give you the most expected configuration set, you can also use a more advanced approach using the state, events and apiRef.

In any case, I think that we should explore all our options. For instance, we could imagine having a state callback selector, allowing developers to cherrypick, sub parts of the state and replicate them from the data grid (being the source of truth in an uncontrolled mode) to a parent state. Isn't what we had onStateChange for?

We already use this approach internally, as we use selectors and useEffect on the gridState.
OnStateChange is here to allow users to intercept the change and do persistence for example, yes it can be used also to control the full state of the grid.
However we have props such as filterModel, sortModel... that give you access to those specific sub part of the state, and with the corresponding onChange handler, it let you manage just those part of the state.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 28, 2021

@dtassone a couple of thoughts:

FYI, we have an event GRID_COLUMNS_UPDATED that is published when columns change. Maybe you could subscribe to this event as a workaround.

  1. Would it make sense to systematically expose events with the props? (With an automatic binding logic). For instance, here, shouldn't we have an onColumnsUpdated prop? These events are already public. It seems that it would a. reduce the bundle size and b. increase consistency.
    For instance, it would allow us to drop this line and it would force us to name the event correctly: GRID_SELECTION_CHANGED -> GRID_SELECTION_MODEL_CHANGE.

https://github.com/mui-org/material-ui-x/blob/570d9123d6200d82ce8920970e646860bdff206e/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts#L162

In the documentation I didn't find any props to show the column parameters and get the columns visible when this event was triggered.

  1. @marcellemcm This is possible with the onStateChange private API and the right selector: https://codesandbox.io/s/material-demo-forked-yc1sh?file=/demo.tsx. The performance aspect of this approach might be lagging a bit behind but it's probably enough for most of the use cases. It seems straightforward:
export default function DataGridDemo() {
  const [visibleColumns, setVisibleColumns] = React.useState();

  console.log("render"); // Only render when the visible columns changes.

  return (
    <div style={{ height: 400, width: "100%" }}>
      <DataGrid
        onStateChange={({ state }) => {
          const newVisibleColumns = visibleGridColumnsSelector(state);
          setVisibleColumns(newVisibleColumns);
        }}
        rows={rows}
        columns={columns}
        pageSize={5}
        checkboxSelection
      />
    </div>
  );
}

It also seems what you wanted to do initially #190 (comment) :p.

However we have props such as filterModel, sortModel... that give you access to those specific sub part of the state, and with the corresponding onChange handler, it let you manage just those part of the state.

  1. The visibility of the columns is currently not controllable under a dedicated state. Not in the way of how it's done in here: https://devexpress.github.io/devextreme-reactive/react/grid/docs/guides/column-visibility/#controlled-mode. I personally think that 1. could be enough to solve the current problem.

  2. Looking back at the discussion we had in [RFC] Change API state #190, and ignoring dead ends I have explored. It seems that the following still holds true (using the edit rows state for illustration):

  • We currently have no true controllable state. This is not right. For instance, https://material-ui.com/components/data-grid/editing/#control-editing fails this concept, proof. It's about setting the source of truth from the grid, to the parent. I don't think that we have the choice, we will need to fix it for the beta phase.
  • We can skip the need for a defaultX prop. This is not that much important as if developers want to set an initial value for a state, they can fully control the prop. By not doing it beforehand, we optimize for learning ad minimize the API surface. Maybe we will get away without it (I doubt it but it would ideal).
  • We need a corresponding imperative API, both for setting the value, and for getting it. It seems to be done correctly. e.g. setEditRowsModel and getEditRowsModel.
  • There is a tradeoff to make. Do we want to scale the controllable state with props (usually two), at the expense how having a very long list of props. Or do we want to find a hybrid approach, for instance useControllableState for allowing to control sub-parts of the state? Or maybe even want to support the two?
  • Do we want to make the onStateChange and state props public?

@dtassone
Copy link
Member

dtassone commented Apr 28, 2021

  1. yes that is part of the pattern. And we are definitely missing consistency here.
  2. yes

We currently have no true controllable state. This is not right. For instance, https://material-ui.com/components/data-grid/editing/#control-editing fails this concept, proof. It's about setting the source of truth from the grid, to the parent. I don't think that we have the choice, we will need to fix it for the beta phase.

Well ATM you need to give a new ref to override the change done by grid.

There is a tradeoff to make. Do we want to scale the controllable state with props (usually two), at the expense how having a very long list of props.

I don't see any issue in having a reasonably long list of props as long as it is well documented.
We could also support the approach of using the state and selector or apiRef selector directly as we do internally.
That could work for components.

 export default function DataGridDemo() {
  const apiRef = useApiRef();
const visibleColumns = useGridSelector(apiRef, visibleGridColumnsSelector);

useEffect(()=> {
//Do something when visible columns changes.
}, [visibleColumns]);

  return (
    <div style={{ height: 400, width: "100%" }}>
      <DataGrid
        rows={rows}
        columns={columns}
        pageSize={5}
        checkboxSelection
      />
    </div>
  );
}

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 28, 2021

Well ATM you need to give a new ref to override the change done by grid.

Agree, this is the problematic behavior I wanted to go after, it's not controlled in the definition of the term, but it's close (IMHO, not close enough).

I don't see any issue in having a reasonably long list of props as long as it is well documented.

Yeah, why not, I don't see a strong reason for not doing it either. Initially, I was defending the idea of having one prop per controllable state. I have only moved my argumentation to the other side so we could keep a healthy discussion between the pros and cons.

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 30, 2021
@peppermint-juli
Copy link

peppermint-juli commented May 3, 2021

@dtassone Loved this approach #1507 (comment), but apiRef is being created as {current = null} it doesn't work

image

@oliviertassinari oliviertassinari changed the title [DataGrid] Get the visible columns after clicking manage columns in the column menu component [DataGrid] Get the visible columns May 5, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid] Get the visible columns [DataGrid] How to get the visible columns? May 5, 2021
@oliviertassinari
Copy link
Member

I'm closing for ##1412 (comment). This should solve the pain.

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

No branches or pull requests

4 participants