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 column order and dimensions to the portable state #3816

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Feb 2, 2022

Closes #3448 (last missing states)

API

type GridColumnDimensionProperties = 'maxWidth' | 'minWidth' | 'width' | 'flex'

type GridColumnDimensions = Pick<GridStateColDef, GridColumnDimensionProperties>;

interface GridColumnsInitialState {
  columnVisibilityModel?: GridColumnVisibilityModel;
  orderedFields?: string[];
  dimensions?: Record<string, GridColumnDimensions>;
}

For consistency, I propose to rename GridState['columns']['all'] into GridState['columns']['orderedFields'] in v6.
The columns selector a not correctly named so we will have a breaking change there anyway.

Examples

Order

Same fields in import and in the columns prop

<DataGrid
  columns={[{ field: 'idBis' }, { field: 'id' }]}
  initialState={{
    columns: {
      orderedFields: ['id', 'idBis']
    }
  }}
/>

console.log(state.columns.all) // ['id', 'idBis']

Missing fields in import

<DataGrid
  columns={[{ field: 'idTres' }, { field: 'idBis' }, { field: 'id' }]}
  initialState={{
    columns: {
      orderedFields: ['id', 'idBis']
    }
  }}
/>

console.log(state.columns.all) // ['id', 'idBis', 'idTres']

Unmatched fields in import

<DataGrid
  columns={{ field: 'idBis' }, { field: 'id' }]}
  initialState={{
    columns: {
      orderedFields: ['id', 'idBis', 'idTres']
    }
  }}
/>

console.log(state.columns.all) // ['id', 'idBis']

@flaviendelangle flaviendelangle added component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine labels Feb 2, 2022
@flaviendelangle flaviendelangle self-assigned this Feb 2, 2022
@mui-bot
Copy link

mui-bot commented Feb 2, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 248.5 490.4 295.3 352.68 102.229
Sort 100k rows ms 458.4 925 680.1 706.82 159.75
Select 100k rows ms 104.1 289.7 141.7 177.8 70.611
Deselect 100k rows ms 113 191.2 173.5 158.2 30.549

Generated by 🚫 dangerJS against 09d1d53

@flaviendelangle flaviendelangle force-pushed the state-columns-import-export branch from 23270c4 to 066aed0 Compare February 2, 2022 10:38
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 2, 2022
@github-actions
Copy link

github-actions bot commented Feb 2, 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 Feb 3, 2022
@siriwatknp
Copy link
Member

siriwatknp commented Feb 8, 2022

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/data-grid/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/x/api/data-grid/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

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

github-actions bot commented Feb 8, 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 Feb 9, 2022
@flaviendelangle flaviendelangle marked this pull request as ready for review February 14, 2022 07:52
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 14, 2022
@github-actions
Copy link

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 Feb 14, 2022
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

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 Mar 1, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 1, 2022
@@ -316,17 +342,23 @@ export function useGridColumns(
return params;
Copy link
Member

Choose a reason for hiding this comment

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

It will return params if columnVisibilityModel and initialState.columns.columnVisibilityModel is not present. That means that to use exportState users need to define these props, even if they are not going to use them. Is this a requirement to use this feature or a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

If they don't use columnVisibilityModel and initialState.columns.columnVisibilityModel but have this model when exporting.
Then they are probably using GridColDef.hide
And if we let them export / re-import the model, I'm not sure the behavior will be coherent.

I would rather add an explicit note on the column visibility legacy doc to say that state import / export requires to use the new api.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is not that we don't support restoring the columns when hide is used. It's not worth to support it since it's a legacy API. The point in my comment was that to able to restore the state with restoreState the user must set columnVisibilityModel to some value. Maybe we should improve the condition used to set shouldUseVisibleColumnModel to assume that we're using the new approach if no column is using hide. Currently, for it to work when I've no intention of controlling the column visibility I need to pass columnVisibilityModel={}.

Copy link
Member Author

@flaviendelangle flaviendelangle Mar 1, 2022

Choose a reason for hiding this comment

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

Oh right I forgot to set shouldRegenColumnVisibilityModelFromColumns to true when using the legacy method instead of the early return.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed
I added a test to make sure that we don't use the model passed to restoreState AND don't loose the current hide: true in state

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

github-actions bot commented Mar 2, 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 Mar 2, 2022
@DanailH
Copy link
Member

DanailH commented Mar 2, 2022

Just a general question and feedback. Since we will also have row reordering #4034 does it make sense to include this feature in the beginning or add it later? Also in terms of its API - should the order be per page if there is pagination enabled?

@flaviendelangle
Copy link
Member Author

@DanailH good question
I propose we wait. Because I don't know how it should behave with server side pagination.

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

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 Mar 4, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 4, 2022
@flaviendelangle
Copy link
Member Author

@m4theushw if we can finish this one

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! feature: Rendering layout Related to the data grid Rendering engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Add other sub-states to the initialState prop
6 participants