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] Fix resizing right pinned column #15107

Merged
merged 13 commits into from
Nov 5, 2024

Conversation

KenanYusuf
Copy link
Member

@KenanYusuf KenanYusuf commented Oct 24, 2024

Fixes #12852 and #13548

There is already a PR open that fixes the same issue, but takes a different approach. Curious to see what others think of this way which relies on always rendering the empty cell (rather than conditionally) to grow/shrink in the space as the column resizes.

Before: https://mui.com/x/react-data-grid/column-pinning/
After: https://deploy-preview-15107--material-ui-x.netlify.app/x/react-data-grid/column-pinning/

@KenanYusuf KenanYusuf added component: data grid This is the name of the generic UI component, not the React module! feature: Column resize labels Oct 24, 2024
@mui-bot
Copy link

mui-bot commented Oct 24, 2024

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

Generated by 🚫 dangerJS against 84ecae7

@KenanYusuf KenanYusuf closed this Oct 24, 2024
@KenanYusuf KenanYusuf reopened this Oct 28, 2024
@@ -477,8 +451,7 @@ const GridRow = React.forwardRef<HTMLDivElement, GridRowProps>(function GridRow(
style={{ width: offsetLeft }}
/>
{cells}
{emptyCellWidth > 0 && <EmptyCell width={emptyCellWidth} />}
{rightCells.length > 0 && <div role="presentation" className={gridClasses.filler} />}
<div role="presentation" className={clsx(gridClasses.cell, gridClasses.cellEmpty)} />
Copy link
Member Author

Choose a reason for hiding this comment

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

Rendering the empty cell all the time improves resizing of the right-pinned cells

Before

before.mov

After

after.mov

Copy link
Member Author

@KenanYusuf KenanYusuf Oct 28, 2024

Choose a reason for hiding this comment

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

Ideally the actions column would also move with the right pinned column...

'--DataGrid-rowWidth',
`${newTotalWidth}px`,
);
if (columnWidthDiff > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Before

before.mov

After

after.mov

@@ -564,6 +564,7 @@ export const GridRootStyles = styled('div', {
lineHeight: 'inherit',
},
[`& .${c.cellEmpty}`]: {
flex: 1,
Copy link
Member Author

Choose a reason for hiding this comment

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

This way the empty cell always takes up the right amount of space in a row. Shrinks when the row is full of cells, grows when there aren't enough to fill a row.

@@ -477,8 +451,7 @@ const GridRow = React.forwardRef<HTMLDivElement, GridRowProps>(function GridRow(
style={{ width: offsetLeft }}
/>
{cells}
{emptyCellWidth > 0 && <EmptyCell width={emptyCellWidth} />}
{rightCells.length > 0 && <div role="presentation" className={gridClasses.filler} />}
Copy link
Member Author

@KenanYusuf KenanYusuf Oct 28, 2024

Choose a reason for hiding this comment

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

Not sure why we need this + the empty cell. Perhaps I'm missing something.

Copy link
Contributor

@romgrk romgrk Oct 29, 2024

Choose a reason for hiding this comment

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

I don't remember :| You should build a test case with all the variations of:

  • pinned or no pinned columns
  • scrollbar or no scrollbar
  • empty space or no empty space (when columns don't fill the width)

And then check if all the cases behave correctly. Make sure to enable vertical borders on all cells, we need to make sure that borders are where they're supposed to be (by default the grid only has vertical border on pinned sections).

We should probably create a visual regression test based on that.

@cherniavskii cherniavskii requested a review from romgrk October 29, 2024 10:01
Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

LGTM, if it behaves the same and fixes all issues that would be amazing.

@KenanYusuf KenanYusuf changed the title [data grid] Fix column resizing issues [DataGrid] Fix resizing right pinned column Oct 30, 2024
@KenanYusuf KenanYusuf marked this pull request as ready for review October 31, 2024 15:50
@KenanYusuf KenanYusuf requested review from a team and removed request for a team November 1, 2024 09:37
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Great job!

packages/x-data-grid/src/tests/rows.DataGrid.test.tsx Outdated Show resolved Hide resolved
KenanYusuf and others added 4 commits November 4, 2024 16:53
@KenanYusuf KenanYusuf merged commit d29b264 into mui:master Nov 5, 2024
18 checks passed
@KenanYusuf KenanYusuf deleted the column-resize-fix branch November 5, 2024 15:30
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] The right pinned column shrinks from the opposite side when resizing
4 participants