[DataGrid] Add sort icon for when column is unsorted #1415

m4theushw · 2021-04-14T14:59:58Z

Closes #1076
Preview: https://deploy-preview-1415--material-ui-x.netlify.app/components/data-grid/demo

This PR changes the usage of the hideSortIcons prop. Using false will display an intermediary icon when no sort is applied. To keep the old behavior I've added another unsorted (default now) value to only show the sort icon when there's a sort applied.

dtassone · 2021-04-14T15:08:50Z

Why do we need the 'unsorted' value exactly?

So does hideSortIcons allow to hide the sort icon for a specific column?

m4theushw · 2021-04-14T15:15:32Z

Why do we need the 'unsorted' value exactly?

unsorted is to not show the intermediary icon when the column is unsorted. It reproduces the current behavior. Here the age column uses it and the name column is false

So does hideSortIcons allow to hide the sort icon for a specific column?

Yep.

packages/grid/_modules_/grid/components/containers/GridRootStyles.ts

dtassone · 2021-04-16T14:17:09Z

hideSortIcons I think this unsorted setting value is really confusing (to me at least).

To summarise:

users can set 3 icons, one for each direction and one when the column is unsorted.
users might want to hide the sort icons for all columns. For this, he can override the icons and put an empty component, or apply hideSortIcons to every sortable columns.
users might want to hide the sort icons for a specific column. Quite specific edge case IMO, but it could do it using hideSortIcons
users might want to hide the unsorted icon for a all columns, in that case, it can just put an empty component in the unsorted icon.
users might want to hide the unsorted icon for a specific column but show it for another one. hideSortIcons='unsorted'. TBH I don't that edge case is necessary. I believe that if users want to hide the unsorted icon, they would do it for all columns, it doesn't make sense to do it for one column and not another. It would be misleading. IMHO, I think we should remove the unsorted option for the reason above. Also the prop name is not ideal as it seems like a bool value, but it can take unsorted.

m4theushw · 2021-04-16T22:27:55Z

users might want to hide the sort icons for all columns. For this, he can override the icons and put an empty component, or apply hideSortIcons to every sortable columns.

If he changes the icon to null/false/undefined the default icon is used. If changed to an empty component (e.g. div) the button is rendered without the icon, but still takes space.

users might want to hide the unsorted icon for a all columns, in that case, it can just put an empty component in the unsorted icon.

Same problem as above.

users might want to hide the unsorted icon for a specific column but show it for another one. hideSortIcons='unsorted'. TBH I don't that edge case is necessary. I believe that if users want to hide the unsorted icon, they would do it for all columns, it doesn't make sense to do it for one column and not another. It would be misleading. IMHO, I think we should remove the unsorted option for the reason above. Also the prop name is not ideal as it seems like a bool value, but it can take unsorted.

The idea of controlling if the unsorted icon should be displayed came from this comment. However, instead of changing to an enum and creating a breaking change, I kept the boolean but added the third option. The Autocomplete API uses this approach in blurOnSelect and forcePopupIcon.

docs/src/pages/components/data-grid/columns/StylingHeaderGrid.js

dtassone · 2021-04-19T13:02:49Z

Remove unsorted value
Sortable columns should be identifiable with the unsorted icon
Icon components should allow null value.

oliviertassinari · 2021-04-19T14:33:02Z

To add a bit to what we have discussed:

The root pain that [DataGrid] Allow sortable indicator on column headers without a sort applied #1076 covers is the accessibility affordance. How can it be made obvious that a column is sortable and another is not?
A new slot for the components prop can solve this issue, it would be rendered for the "unsorted" state on columns that are sortable.
Is there really a use case for hiding the sort icons? Why not disable the sort on the column in the first place? How about we remove the hideSortIcons property?
Accepting null for a component slot as a way to disable it could be an exception, we have never done this before, we shouldn't generalize it.

packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderItem.tsx

dtassone · 2021-04-23T14:03:47Z

Issue on this one
Sort icon should be displayed on the right for boolean
Should we show the unsorted icon by default? If so, this icon does not mean unsorted but sorted desc. I would pick either importExport or unfoldMore from the MUI set. We could also apply an hover style which would increase the opacity to make it less intrusive.
We used to always show the menu icon on top of all icons, as it allows to do any actions, there is a small regression.
now

before

as a side note, this column menu demo is really bad. It just shows how to disable it
https://deploy-preview-1415--material-ui-x.netlify.app/components/data-grid/columns/#column-menu

m4theushw · 2021-04-23T14:48:47Z

Issue on this one

Fixed.

Sort icon should be displayed on the right for boolean

Fixed.

Should we show the unsorted icon by default? If so, this icon does not mean unsorted but sorted desc. I would pick either importExport or unfoldMore from the MUI set. We could also apply an hover style which would increase the opacity to make it less intrusive.

I changed to importExport but I'm still not happy. I think we should only show the icon on hover and use the same icon of the next sort direction. #1076 (comment)

We used to always show the menu icon on top of all icons, as it allows to do any actions, there is a small regression.

I related this problem here. It's a trade-off of fixing the alignment when using headerAlign=center. We can add a background but that's a visual breaking change.

dtassone · 2021-04-23T16:53:06Z

I related this problem here. It's a trade-off of fixing the alignment when using headerAlign=center. We can add a background but that's a visual breaking change.

Yes but the tradeoff seems that it hurts more than the issue. Have you tried with a negative margin?

I changed to importExport but I'm still not happy. I think we should only show the icon on hover and use the same icon of the next sort direction. #1076 (comment)

Well the issue is to provide the ability to show the unsorted icon when the column is not sorted. If we show the next direction then it would be a different spec. It could be a new option on sortable columns. Something like showNextDirectionIconOnSortableColumns, but that would be a different feature. Demo candidate in doc? So we show the customizability of the feature or we wait for a user to create the issue.

For the current issue, we have 2 default options.

we leave the unsorted icon null.
we put an unsorted icon and apply a css to show it on hover.

As the grid should always come with as many feature as possible, I would lean towards 2.
For css I would go with a low opacity to show it in grey as it's just an info, compared to the active sort state in black.

m4theushw · 2021-04-23T18:08:20Z

Yes but the tradeoff seems that it hurts more than the issue. Have you tried with a negative margin?

Same problem. I applied a negative margin-right to the .MuiDataGrid-colCellTitleContainer class of the demo here. Since the column menu is transparent, everything below it is visible.

Well the issue is to provide the ability to show the unsorted icon when the column is not sorted. If we show the next direction then it would be a different spec. It could be a new option on sortable columns. Something like showNextDirectionIconOnSortableColumns, but that would be a different feature. Demo candidate in doc? So we show the customizability of the feature or we wait for a user to create the issue.

We added the icon slot for the unsorted icon, users can use any icon they want. My point is which icon should we use as the default one. Since we don't have a proper one my idea was to reproduce the same behavior of #1076 (comment) here. I just added now the GridColumnUnsortedIcon which is a "dynamic icon", showing the up arrow icon or the down arrow icon, depending on the sortingOrder. It can be used as the unsorted icon. I'll still have to fix the CSS to increase its alpha when the column is not sorted.

oliviertassinari

How about we document this in https://deploy-preview-1415--material-ui-x.netlify.app/components/data-grid/sorting/?
It seems that there are broken styles. I don't understand why we use a translateX. This is counterintuitive to me.

docs/pages/api-docs/data-grid.md

docs/pages/api-docs/x-grid.md

packages/grid/_modules_/grid/components/containers/GridRootStyles.ts

dtassone · 2021-04-26T13:10:22Z

As discussed

let's restore the alignment issue
fix the opacity

m4theushw · 2021-04-26T18:02:12Z

packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderItem.tsx

      <ColumnHeaderFilterIcon counter={filterItemsCounter} />
+      {column.sortable && !column.hideSortIcons && (


I had to invert the filter icon and the sort icon or it gonna have an empty space when the column is unsorted, but there's an active filter.

m4theushw · 2021-04-26T18:02:55Z

packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderSortIcon.tsx

+  } else if (direction === 'desc') {
+    Icon = icons!.ColumnSortedDescendingIcon!;
+  }
+  return <Icon fontSize="small" className="MuiDataGrid-sortIcon" />;


Added fontSize to match with ColumnHeaderFilterIcon

oliviertassinari

Outside of the font size regression?, it looks great.

packages/grid/_modules_/grid/components/containers/GridRootStyles.ts

Opps, I have fired too quicky. There are visual regressions in the demos. For instance, here https://deploy-preview-1415--material-ui-x.netlify.app/components/data-grid/#mit-version, we need a larger width

dtassone · 2021-05-03T10:40:20Z

There are also the issues with the icon on top of the other mentioned
#1415 (comment)

oliviertassinari · 2021-05-03T10:50:01Z

There are also the issues with the icon on top of the other mentioned #1415 (comment)

@dtassone I don't follow. What's the issue? Could you provide enough information on how to reproduce bugs when you report them?

dtassone · 2021-05-03T10:52:38Z

There are also the issues with the icon on top of the other mentioned #1415 (comment)

@dtassone I don't follow. What's the issue? Could you provide enough information on how to reproduce bugs when you report them?

As mentioned in the link I put, if you reduce the size of the columns then the icons overlap each others

We used to always show the menu icon on top of all icons, as it allows to do any actions, there is a small regression.
now

before

m4theushw · 2021-05-03T11:17:35Z

As mentioned in the link I put, if you reduce the size of the columns then the icons overlap each others

@dtassone That's a side effect of removing overflow: hidden from .MuiDataGrid-colCellTitleContainer. It was removed because the value in the badge was being cut when reducing the size of the column. I can't think of a way to stack the column menu without adding the background.

With overflow: hidden:

oliviertassinari · 2021-05-03T11:27:25Z

As mentioned in the link I put

The only link I can find is https://deploy-preview-1415--material-ui-x.netlify.app/components/data-grid/columns/#column-menu. I personally can't see any issue. The columns are not resizable on these demos.

I can see something wrong in https://deploy-preview-1415--material-ui-x.netlify.app/components/data-grid/columns/#column-reorder.

dtassone · 2021-05-03T12:08:20Z

try overriding the style below.

.MuiBadge-anchorOriginTopRightRectangle {
    top: 0;
    right: 0;
    transform: scale(1) translate(30%, -50%);  <= the fix is here, change from 50 to 30%.
    transform-origin: 100% 0%;
}

m4theushw · 2021-05-03T14:52:43Z

try overriding the style below.

@dtassone Same problem.

If we invert the horizontal anchor of the Badge we can the solve the regression where the sort icon overlaps the column menu.

dtassone · 2021-05-03T14:55:12Z

try overriding the style below.

@dtassone Same problem.

you have to restore the overflow and your changes

oliviertassinari · 2021-05-03T23:57:10Z

If we invert the horizontal anchor of the Badge we can the solve the regression where the sort icon overlaps the column menu.

@m4theushw It does feel better too. We would only need to make sure it works if the text alignment is reversed.

Otherwise, we could reduce the padding of .MuiDataGrid-colCell and add some of it on .MuiDataGrid-colCellTitleContainer, so that the overflow hidden doesn't hide the badge.

DanailH · 2021-05-12T11:48:30Z

@m4theushw you would need to accept the new screenshots to fix the build 😉

WilliamZimmermann · 2021-09-24T13:42:07Z

Hello guys. Finally, the feature to "add sort icon for when column is unsorted" (for component) is not implemented? Because I can't see in the docs (for previous version [4] or the new version [5]) any references to it. If it is not implemented and will not be implemented soon, do you have any suggestion of workaround? Thx!

m4theushw commented Apr 14, 2021

View reviewed changes

packages/grid/_modules_/grid/components/containers/GridRootStyles.ts Outdated Show resolved Hide resolved

DanailH reviewed Apr 19, 2021

View reviewed changes

docs/src/pages/components/data-grid/columns/StylingHeaderGrid.js Outdated Show resolved Hide resolved

m4theushw force-pushed the sort-icon branch from 45c9171 to d9964d8 Compare April 20, 2021 01:10

dtassone reviewed Apr 23, 2021

View reviewed changes

packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderItem.tsx Outdated Show resolved Hide resolved

m4theushw mentioned this pull request Apr 23, 2021

[DataGrid] Allow sortable indicator on column headers without a sort applied #1076

Closed

1 task

oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Apr 25, 2021

oliviertassinari requested changes Apr 25, 2021

View reviewed changes

docs/pages/api-docs/data-grid.md Outdated Show resolved Hide resolved

docs/pages/api-docs/x-grid.md Outdated Show resolved Hide resolved

packages/grid/_modules_/grid/components/containers/GridRootStyles.ts Outdated Show resolved Hide resolved

m4theushw force-pushed the sort-icon branch from cf0a23d to 59ce94a Compare April 26, 2021 17:18

m4theushw commented Apr 26, 2021

View reviewed changes

oliviertassinari previously approved these changes May 2, 2021

View reviewed changes

packages/grid/_modules_/grid/components/containers/GridRootStyles.ts Show resolved Hide resolved

m4theushw added 3 commits May 3, 2021 08:53

[DataGrid] Add icon for when column is unsorted

98b2f9a

change icon font size

14c82d7

increase column widths

348d9f9

m4theushw force-pushed the sort-icon branch from 59ce94a to 348d9f9 Compare May 3, 2021 12:02

add "overflow: hidden" back

eae9d53

oliviertassinari added the new feature New feature or request label May 8, 2021

rerun ci, looks good

df7f525

DanailH approved these changes May 10, 2021

View reviewed changes

Merge branch 'master' into sort-icon

d6ec412

m4theushw mentioned this pull request May 11, 2021

[DataGrid] Clicking on the filter icon in header title is not opening the filter panel #1618

Closed

Merge branch 'master' into sort-icon

7c42226

dtassone approved these changes May 12, 2021

View reviewed changes

m4theushw added 2 commits May 13, 2021 13:36

Merge branch 'master' into sort-icon

8a17aab

small typo

bd17e9c

m4theushw merged commit 2a3779c into mui:master May 13, 2021

m4theushw deleted the sort-icon branch May 13, 2021 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Add sort icon for when column is unsorted #1415

[DataGrid] Add sort icon for when column is unsorted #1415

m4theushw commented Apr 14, 2021 •

edited

dtassone commented Apr 14, 2021

m4theushw commented Apr 14, 2021 •

edited

dtassone commented Apr 16, 2021 •

edited

m4theushw commented Apr 16, 2021 •

edited

dtassone commented Apr 19, 2021

oliviertassinari commented Apr 19, 2021 •

edited

dtassone commented Apr 23, 2021

m4theushw commented Apr 23, 2021

dtassone commented Apr 23, 2021 •

edited

m4theushw commented Apr 23, 2021 •

edited

oliviertassinari left a comment •

edited

dtassone commented Apr 26, 2021

m4theushw Apr 26, 2021

m4theushw Apr 26, 2021

oliviertassinari left a comment

dtassone commented May 3, 2021

oliviertassinari commented May 3, 2021 •

edited

dtassone commented May 3, 2021

m4theushw commented May 3, 2021

oliviertassinari commented May 3, 2021 •

edited

dtassone commented May 3, 2021

m4theushw commented May 3, 2021

dtassone commented May 3, 2021

oliviertassinari commented May 3, 2021 •

edited

DanailH commented May 12, 2021 •

edited by m4theushw

WilliamZimmermann commented Sep 24, 2021

		<ColumnHeaderFilterIcon counter={filterItemsCounter} />
		{column.sortable && !column.hideSortIcons && (

[DataGrid] Add sort icon for when column is unsorted #1415

[DataGrid] Add sort icon for when column is unsorted #1415

Conversation

m4theushw commented Apr 14, 2021 • edited

dtassone commented Apr 14, 2021

m4theushw commented Apr 14, 2021 • edited

dtassone commented Apr 16, 2021 • edited

m4theushw commented Apr 16, 2021 • edited

dtassone commented Apr 19, 2021

oliviertassinari commented Apr 19, 2021 • edited

dtassone commented Apr 23, 2021

m4theushw commented Apr 23, 2021

dtassone commented Apr 23, 2021 • edited

m4theushw commented Apr 23, 2021 • edited

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

dtassone commented Apr 26, 2021

m4theushw Apr 26, 2021

Choose a reason for hiding this comment

m4theushw Apr 26, 2021

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

dtassone commented May 3, 2021

oliviertassinari commented May 3, 2021 • edited

dtassone commented May 3, 2021

m4theushw commented May 3, 2021

oliviertassinari commented May 3, 2021 • edited

dtassone commented May 3, 2021

m4theushw commented May 3, 2021

dtassone commented May 3, 2021

oliviertassinari commented May 3, 2021 • edited

DanailH commented May 12, 2021 • edited by m4theushw

WilliamZimmermann commented Sep 24, 2021

m4theushw commented Apr 14, 2021 •

edited

m4theushw commented Apr 14, 2021 •

edited

dtassone commented Apr 16, 2021 •

edited

m4theushw commented Apr 16, 2021 •

edited

oliviertassinari commented Apr 19, 2021 •

edited

dtassone commented Apr 23, 2021 •

edited

m4theushw commented Apr 23, 2021 •

edited

oliviertassinari left a comment •

edited

oliviertassinari commented May 3, 2021 •

edited

oliviertassinari commented May 3, 2021 •

edited

oliviertassinari commented May 3, 2021 •

edited

DanailH commented May 12, 2021 •

edited by m4theushw