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 recipe to persist column width and order #14560

Merged
merged 14 commits into from
Nov 7, 2024

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Sep 10, 2024

Resolves #14452

Approach: Using the computedWidth value from the previous state reference at column width hydration.

A similar approach could technically be applied for column reordering by computing the order of columnsState.orderedFields based on the previous state reference.

https://deploy-preview-14560--material-ui-x.netlify.app/x/react-data-grid/column-dimensions/#persisting-column-width

New approach: #14560 (comment)

Preview: https://deploy-preview-14560--material-ui-x.netlify.app/x/react-data-grid/column-recipes/#persisting-column-width-and-order

@MBilalShafi MBilalShafi added component: data grid This is the name of the generic UI component, not the React module! feature: Column resize proof of concept Studying and/or experimenting with a to be validated approach labels Sep 10, 2024
@mui-bot
Copy link

mui-bot commented Sep 10, 2024

Deploy preview: https://deploy-preview-14560--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against 3afe7b4

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love how this approach works. I feel like we're complexifying the inner state of the grid even more, and it's already too complex and in need of a refactor. I wonder if there wouldn't be a way to provide a recipe/export hook that would handle this logic outside the grid?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure exporting a hook would make things less complex.
IMO this solution is OK given the number of issues we get regarding this.
I'm in favor of merging it, unless we have a better short-term solution.

Copy link
Member Author

@MBilalShafi MBilalShafi Sep 16, 2024

Choose a reason for hiding this comment

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

It seems a quick win to me with the least amount of learning curve on the user side, once we get a chance to refactor the state, we might approach that in a better way. We could add a follow-up issue so we don't forget about it. Wdyt?


Tbh, this specific issue is related to our design choice with the column definitions. i.e. treating the columns prop update as a new set of columns, and this solution is just an escape-hatch tailored per users' requirement.

One way to work that around in the long term could be to treat the columns prop as initalState.something (no more need to stabilize its reference).
The users would then use an imperative API if an update to some colDef is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

One way to work that around in the long term could be to treat the columns prop as initalState.something (no more need to stabilize its reference).
The users would then use an imperative API if an update to some colDef is needed.

Should we immediately deprecate this flag and in the upcoming major set the new behavior as default?

From my perspective, we should keep all user interactions intact between re-renders. Same is now done with sorting, filtering, pagination, etc. So I don't see why the column resize should be treated differently (setting aside the technical implementation)

Technically, we could rely on the column id and restore the updates even when they are reordered or if some other unrelated props are updated

Copy link
Contributor

@romgrk romgrk Sep 18, 2024

Choose a reason for hiding this comment

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

From my perspective, we should keep all user interactions intact between re-renders. Same is now done with sorting, filtering, pagination, etc

Not really the same, those models are either controlled or uncontrolled. Columns are the only model that is semi-controlled.

Copy link
Member

Choose a reason for hiding this comment

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

@romgrk This looks promising!
columnState.update would track the column width and order states and apply them to the provided column definitions, correct?
Also, how about passing column definitions to the useColumnState hook directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the hook could track width & order. And yes, we can also refactor it further to make it more ergonomic, I just wanted to illustrate how it can work:

const columnState = useColumnState(useMemo(() => [
  { field: 'id', width: 100 },
  { field: 'username' },
  { field: 'email' },
], []))

<DataGrid
  columns={columnState.columns}
  onColumnWidthChange={columnState.onWidthChange}
  onColumnOrderChange={columnState.onOrderChange}
/>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for sharing the interface @romgrk. I'll try to POC it.

On first impression, it seems we might also have to replicate some internal logic or do some refactoring to it because on the first render when it does internal computations like for the flex columns, it doesn't call onColumnWidthChange. We don't have a reactive state binding option for the export hook too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to have the flex width? The goal is only to persist resize/reorder, so I think using the events as they are should be enough. The first resize event will contain the correct width to persist, and before that there is no need to persist it, the grid will do its thing as it does now.

Copy link
Member Author

@MBilalShafi MBilalShafi Nov 6, 2024

Choose a reason for hiding this comment

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

Is it really necessary to have the flex width?

I think we can start without it and wait for the users to request a specific need for it. The idea seems to work great: https://stackblitz.com/edit/react-l6w6ni?file=Demo.tsx 👍

I'll update this PR with this recipe instead of the internal implementation.

Updated: https://deploy-preview-14560--material-ui-x.netlify.app/x/react-data-grid/column-recipes/#persisting-column-width-and-order

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 20, 2024

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 28, 2024
@MBilalShafi MBilalShafi changed the title [DataGrid] POC: Allow to persist column width [DataGrid] Add option to persist column width Sep 28, 2024
@MBilalShafi MBilalShafi added enhancement This is not a bug, nor a new feature and removed proof of concept Studying and/or experimenting with a to be validated approach labels Sep 28, 2024
@MBilalShafi MBilalShafi marked this pull request as ready for review September 28, 2024 12:53
@MBilalShafi MBilalShafi requested a review from a team October 11, 2024 13:34
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 15, 2024

This comment was marked as outdated.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 16, 2024

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 17, 2024
@cherniavskii cherniavskii self-requested a review October 28, 2024 13:05
@MBilalShafi MBilalShafi changed the title [DataGrid] Add option to persist column width [DataGrid] Add a recipe to persist column width and order Nov 6, 2024
@MBilalShafi MBilalShafi added recipe and removed enhancement This is not a bug, nor a new feature labels Nov 6, 2024
@MBilalShafi MBilalShafi added the needs cherry-pick The PR should be cherry-picked to master after merge label Nov 6, 2024
@MBilalShafi MBilalShafi enabled auto-merge (squash) November 6, 2024 17:50
@MBilalShafi MBilalShafi merged commit 7061865 into mui:master Nov 7, 2024
20 checks passed
MBilalShafi added a commit to MBilalShafi/mui-x that referenced this pull request Nov 7, 2024
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: Column resize needs cherry-pick The PR should be cherry-picked to master after merge recipe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Can not preserve updated width of columns after resizing
5 participants