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 last column border inconsistency #4224

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Mar 18, 2022

Fix #4200

Why headers and row behave differently

The inconsistency between header and rows comes from the computation of the last column, which should be modified as follow

-isLastColumn={columnIndex === columns.length - 1}
+isLastColumn={columnIndex === visibleColumns.length - 1}

It appears in the issue example because one of the columns was hidden

Proposed solution

In #2444 the consistency has been added, and the chosen solution was to remove borders for the last column. I propose to do the opposite for two reasons:

  • It seems that at that time if the last column was flex, it displayed a double border. One for the grid container, and one for the cell border. This problem does not occur now
  • In the Pro version, we need handlers to resize columns. If showColumnRightBorder=true this handlers became invisible, and so we need a border to know exactly where doe the last column ends

@mui-bot
Copy link

mui-bot commented Mar 18, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 242.4 577.3 308.3 383.26 132.209
Sort 100k rows ms 449.9 994.8 701.5 716.8 217.351
Select 100k rows ms 120.8 206.9 160.1 165.16 32.557
Deselect 100k rows ms 115.4 210.5 161.7 166.22 33.837

Generated by 🚫 dangerJS against e1b7350

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Shouldn't we fix disableExtendRowFullWidth too? Reading its description seems that we're supporting partially it = only removing the last border. If the description is correct, disableExtendRowFullWidth=false should behave like if the last column had flex=1.

* If `true`, rows will not be extended to fill the full width of the grid container.
* @default false
*/
disableExtendRowFullWidth: boolean;

Currently, it gives the impression that the row is extending the full width, but once the cell is clicked I see its true width.

image

Changing the way disableExtendRowFullWidth works can be a breaking change, it depends on how we see its behavior.

const showRightBorder = !isLastColumn
? rootProps.showCellRightBorder
: !removeLastBorderRight && rootProps.disableExtendRowFullWidth;
const showRightBorder = rootProps.showCellRightBorder;
Copy link
Member

@m4theushw m4theushw Mar 23, 2022

Choose a reason for hiding this comment

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

I think part of the previous code we should keep. On OSs that have 0px width scrollbar (e.g. macOS or mobile), I can still see the border.

msedge_WI93JzhmFc

@alexfauquette
Copy link
Member Author

@m4theushw I reverted my changes, to only correct the isLastColumn computation such that rows and columns have the same behavior

About disableExtendRowFullWidth=false I gave it a try, but I did not succeed, because I tried to do the modifications in components rendering, style. But they are overridden when resizing.

In fact their is multiple level of implementations:

  • At the initialisation the with is considered as a min-width for the last column
  • The last column always fill the remaining space

But this sounds like a low priority feature. As soon as the grid overflow in x-axis, it does not have any impact and I believe it's the most frequent case

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

We can continue the discussion for deeper changes later on
But this one-liner seems to fix an error so I would be in favor of moving forward

@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Mar 29, 2022
@alexfauquette alexfauquette merged commit dde149d into mui:master Mar 29, 2022
@alexfauquette alexfauquette deleted the fix-last-border branch March 29, 2022 12:42
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

About disableExtendRowFullWidth=false I gave it a try, but I did not succeed, because I tried to do the modifications in components rendering, style. But they are overridden when resizing.

In v6 we can revisit this prop and make it work correctly. What about adding it to #3287?

@alexfauquette
Copy link
Member Author

@m4theushw I added it

alexfauquette added a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Last column doesn't have a right border
4 participants