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

[data grid] Remove Unsort from the column menu if sortingOrder is ['asc', 'desc'] #7103

Closed
2 tasks done
Valentin1918 opened this issue Dec 6, 2022 · 8 comments · Fixed by #7125
Closed
2 tasks done
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Sorting Related to the data grid Sorting feature good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@Valentin1918
Copy link

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example:

Steps:
1.
2.
3.

Current behavior 😯

Column menu is not reacted on sortingOrder property passed to grid component

Expected behavior 🤔

Column menu sorting items (Unsort, Sort by ASC, Sort by DESC) should be filtered by sortingOrder passed in a data grid component as well as in each column separately

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Order ID 💳 (optional)

No response

@Valentin1918 Valentin1918 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 6, 2022
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Dec 6, 2022
@cherniavskii
Copy link
Member

Hey @Valentin1918
Thanks for reporting this, it looks like a bug.
I can reproduce it with this demo: https://codesandbox.io/s/gallant-lederberg-3n3hx1?file=/demo.tsx
Screenshot 2022-12-06 at 12 27 55

By the way, we are working on the new version of the column menu #6619
We should avoid this issue in the new column menu if possible. cc @MBilalShafi

@cherniavskii cherniavskii added bug 🐛 Something doesn't work feature: Sorting Related to the data grid Sorting feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 6, 2022
@cherniavskii
Copy link
Member

For the current column menu implementation, the fix would look like this:

diff --git a/packages/grid/x-data-grid/src/components/menu/columnMenu/SortGridMenuItems.tsx b/packages/grid/x-data-grid/src/components/menu/columnMenu/SortGridMenuItems.tsx
index ac345d2..d4380dc 100644
--- a/packages/grid/x-data-grid/src/components/menu/columnMenu/SortGridMenuItems.tsx
+++ b/packages/grid/x-data-grid/src/components/menu/columnMenu/SortGridMenuItems.tsx
@@ -6,11 +6,13 @@ import { gridSortModelSelector } from '../../../hooks/features/sorting/gridSorti
 import { GridSortDirection } from '../../../models/gridSortModel';
 import { useGridApiContext } from '../../../hooks/utils/useGridApiContext';
 import { GridFilterItemProps } from './GridFilterItemProps';
+import { useGridRootProps } from '../../../hooks/utils/useGridRootProps';
 
 function SortGridMenuItems(props: GridFilterItemProps) {
   const { column, onClick } = props;
   const apiRef = useGridApiContext();
   const sortModel = useGridSelector(apiRef, gridSortModelSelector);
+  const rootProps = useGridRootProps();
 
   const sortDirection = React.useMemo(() => {
     if (!column) {
@@ -33,17 +35,42 @@ function SortGridMenuItems(props: GridFilterItemProps) {
     return null;
   }
 
+  const sortingOrder: GridSortDirection[] = column.sortingOrder ?? rootProps.sortingOrder;
+
   return (
     <React.Fragment>
-      <MenuItem onClick={onSortMenuItemClick} disabled={sortDirection == null}>
-        {apiRef.current.getLocaleText('columnMenuUnsort')}
-      </MenuItem>
-      <MenuItem onClick={onSortMenuItemClick} data-value="asc" disabled={sortDirection === 'asc'}>
-        {apiRef.current.getLocaleText('columnMenuSortAsc')}
-      </MenuItem>
-      <MenuItem onClick={onSortMenuItemClick} data-value="desc" disabled={sortDirection === 'desc'}>
-        {apiRef.current.getLocaleText('columnMenuSortDesc')}
-      </MenuItem>
+      {sortingOrder.map((direction) => {
+        if (direction === 'asc') {
+          return (
+            <MenuItem
+              onClick={onSortMenuItemClick}
+              data-value="asc"
+              disabled={sortDirection === 'asc'}
+            >
+              {apiRef.current.getLocaleText('columnMenuSortAsc')}
+            </MenuItem>
+          );
+        }
+        if (direction === 'desc') {
+          return (
+            <MenuItem
+              onClick={onSortMenuItemClick}
+              data-value="desc"
+              disabled={sortDirection === 'desc'}
+            >
+              {apiRef.current.getLocaleText('columnMenuSortDesc')}
+            </MenuItem>
+          );
+        }
+        if (direction === null) {
+          return (
+            <MenuItem onClick={onSortMenuItemClick} disabled={sortDirection == null}>
+              {apiRef.current.getLocaleText('columnMenuUnsort')}
+            </MenuItem>
+          );
+        }
+        return null;
+      })}
     </React.Fragment>
   );
 }

@cherniavskii cherniavskii added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 6, 2022
@m4theushw m4theushw changed the title Remove Unsort from the column menu if sortingOrder is ['asc', 'desc'] [data grid] Remove Unsort from the column menu if sortingOrder is ['asc', 'desc'] Dec 6, 2022
@hanbin9775
Copy link
Contributor

Hi! Seems like @cherniavskii already suggested fix for this issue, but could I create a pr for this issue with a bit of code refactoring?

@cherniavskii
Copy link
Member

@hanbin9775 Sure, feel free to submit a PR!

@hanbin9775
Copy link
Contributor

Hi @MBilalShafi, saw your commit about this issue's fix and have a question about it.
It seems that, it would render the menu items in a different order than the code suggested by @cherniavskii.
Are there any specific rules with ordering menu items?

@Valentin1918
Copy link
Author

Hello @hanbin9775 , according to this PR, looks like the order will be according sortingOrder. Order of IF statements inside a map is not important. So it looks good )

@Valentin1918
Copy link
Author

Sorry, I was speaking about this code snippet #7103 (comment) -- it looks good. Concerning PR -- it doesn't take into account an order of sortingOrder

@MBilalShafi
Copy link
Member

Are there any specific rules with ordering menu items?

Yes, we need to have a specific order of the column menu items irrespective of the order in sortingOrder. sortingOrder determines the order in which sorting should be applied not the positioning of items in the menu. As a side effect of changing the order of items unnecessarily, you had to change the test here too.

This should be the order of sort items in the simple column menu:

  1. Unsort
  2. Ascending
  3. Descending

You can use something similar to this to achieve that.

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! feature: Sorting Related to the data grid Sorting feature good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
5 participants