[DataGrid] Remove column separator to match table styles

[MBilalShafi]

Fixes #1623

Removed column separator, it will only be shown when there's resize support available.

Preview: https://deploy-preview-7067--material-ui-x.netlify.app/x/react-data-grid/

Changelog

• Remove column separator
• Remove unused disableHeaderSeparator

Design

• Figma Process
• Figma Final

[mui-bot]

	Messages
📖	Netlify deploy preview: https://deploy-preview-7067--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	644.9	1,094.7	644.9	853.68	192.701
Sort 100k rows	ms	674.1	1,163.5	1,163.5	983.38	183.093
Select 100k rows	ms	199.6	369.8	310.9	292.94	56.365
Deselect 100k rows	ms	159.3	224.8	200.8	197.5	24.451

Generated by 🚫 dangerJS against ea20b88

[MBilalShafi]

@mui/xgrid Team, I couldn't find this prop being referenced or used anywhere, is it already deprecated and is waiting to be removed or there is something else to it?

[cherniavskii]

It was added in #5133 for column grouping. It's used internally since the component is not exported:

mui-x/packages/grid/x-data-grid/src/components/columnHeaders/GridColumnGroupHeader.tsx

Line 193 in d8b76d3

disableHeaderSeparator

[cherniavskii]

I agree it's not needed anymore, since the separator will only be rendered for resizable columns and column groups are not resizable (at least for now).

[cherniavskii]

Looks good to me!

[joserodolfofreitas]

Last Friday, I think I saw the separator appear on hover, but now the separator seems to be gone entirely. Is that intentional?

I think the ideal, would be to keep the separators on hover and display a separator on both sides of the header. Would that be possible? (Initially, the separator is only on the right border).

[cherniavskii]

@joserodolfofreitas the separator appears on hover, but only if the column is resizable. Do you think showing separators for non-resizable columns would be useful? It won't be possible to resize the column anyway.

[joserodolfofreitas]

the separator appears on hover, but only if the column is resizable.
Do you think showing separators for non-resizable columns would be useful?

Ah ok. In this case, I think It's fine to show the separator only when it's resizable.

[joserodolfofreitas]

I think It's fine to show the separator only when it's resizable.

Independently of that, I think we should explore showing the separator on both sides when we do show them.

[m4theushw]

The styles from

mui-x/packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts

Line 216 in d2cf267

[`& .${gridClasses['columnSeparator--resizable']}`]: {

could be merged into

mui-x/packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts

Line 202 in d2cf267

[`& .${gridClasses.columnSeparator}`]: {

The MuiDataGrid-columnSeparator--resizable CSS class is not necessary anymore.

[m4theushw]

I wonder if this change will not create a learning curve for first time users. The separator always visible clearly states to which column the menu icon belongs to. Since it's gone now, the menu icon may appear to belong to the wrong column. Maybe the menu icon should be rendered immediately after the sort/filter icons.

[MBilalShafi]

Maybe the menu icon should be rendered immediately after the sort/filter icons.

Thanks for raising this pain point, yes I also feel this could cause a bit of confusion for the user. Rendering it immediately after sort/filter could be a solution, but should this behavior be the same when resize support is available? Not sure how will it look like then.

[MBilalShafi]

This is how it looks when everything is enabled:

[MBilalShafi]

I think the ideal, would be to keep the separators on hover and display a separator on both sides of the header.

That's a good idea though IMO the user might have difficulty understanding the boundaries of columns for two separators, especially when numeric and non-numeric columns are mixed up.

Another possibility is to show all separators on hover, I feel it looks better as well as it doesn't show separators when not needed. What do you think?

Here are the single and all variants compared:
With one separator: https://codesandbox.io/s/column-separators-variant-1-ujn042?file=/demo.tsx
With all separators: https://codesandbox.io/s/column-separators-variant-2-ly0i1k?file=/demo.tsx

CC @gerdadesign

[cherniavskii]

@MBilalShafi Thanks for the examples!
I really like the demo with all separators on hover!
I've noticed a few things related to the menu icon position change:

1. The menu icon button is cut:

2. Menu icon jump looks a bit odd:

Screen.Recording.2022-12-13.at.11.22.49.mov

3. In MIT DataGrid it's sometimes unclear which column the menu button belongs to.
   In the example below, one might think the menu button would open the column menu for the Trader Email column:

Maybe we should consider showing all separators on hover on MIT DataGrid as well while keeping the original position of the menu icon?

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[gerdadesign]

+1 for displaying all separators on hover.
I also see how it can be confusing to know which column the actions are attached to on hover.

If we stuck with keeping the icons where they are now and showing all separators on hover, we might be able to use the separator type to distinguish between resizeable and not resizable. Perhaps we could think about using a vertical divider that matches the row divider in style for not resizing and adding the small separator as a "handle"? visual illustration below

[MBilalShafi]

Perhaps we could think about using a vertical divider that matches the row divider in style for not resizing and adding

Using a vertical divider is a good idea and it looks great too

The only issue is, it doesn't fit great with the toolbar layout

Here's a live version: https://codesandbox.io/s/column-separators-variant-3-u2o9w5?file=/demo.tsx

Though adding a border on top may make some sense, I feel it ends up being too cluttered.

Maybe only showing the disabled resize handle on the MIT data grid on hover could be the best way forward?

[gerdadesign]

Here's a live version: https://codesandbox.io/s/column-separators-variant-3-u2o9w5?file=/demo.tsx

I like this direction! It does feel incomplete with the toolbar, but without a top border. Is it possible to add that to the hover style of the header row? May feel cluttered if it's on all the time, but if only on hover, it's temporary and provides a clear separation.

[joserodolfofreitas]

I like the explorations going on here, but I think it's worth doing on its own issue as it proposes to solve a different problem related to column resizing UI/UX.

Overall, I believe the solution we reached to show all separators on hover feels like a good output for the problem we proposed ourselves to deal with here.

[oliviertassinari]

Great to see movement on this, it feels like a step forward.

---

On https://deploy-preview-7067--material-ui-x.netlify.app/x/react-data-grid/. I think that hiding the separator when the column isn't resizable would be another step forward. Personally, when I see:

I interpret the separator as: I can resize this column, but then when I try, I can't. #7067 (comment) might feel even better.

[cherniavskii]

@oliviertassinari Showing separators on hover when the column isn't resizable solves (3) in #7067 (comment). Without separators, it's pretty confusing.

[oliviertassinari]

@cherniavskii I think that the experience with the column menu in (3) would be less confusing than the experience on HEAD when it comes to resizing, but it's only based on one end-user (me) that tried the change with CSS and didn't experience point (3).

In any cases, assuming we move forward with something in the direction of #7067 (comment): no separator by default, a drag and drop handle on hover, a subtle separator between columns on hover, then end-users might have a great UX. Is there a follow up planned?

[MBilalShafi]

Is there a follow up planned?

Yes, we'll try to address it with this issue: #7268.