[data grid] Separate ASC and DESC GridComparatorFn

[tomatblackcape]

Summary

I would like the ability to specify a separate GridComparatorFn for sorting columns ASC and DESC, or alternatively an optional third parameter to the GridComparatorFn specifying the sort order so the logic can switch.

Examples

This is the current behavior. I would like the dashes on the right image to remain below the numbers:

Motivation

I have a column of numeric values that may be null, and the desired behavior is for all numeric values to appear before all null values, but sorted ASC or DESC as appropriate. For this I need an "asymmetric" sort function.

If there's a way to do it with the current version of DataGridPro, please let me know!

Search keywords: datagrid column sort custom Order ID: 83510

[michelengelen]

Hey @tomatblackcape ... I have played with this a bit and I am not sure if we can provide such a behavior as of now. There is the option to built a custom comparator to sort the null values last (example here: custom comparator).

The problem that we cannot solve with this is the following: in order for us to provide an easy option to sort by asc, desc and null (unsorted) we just add a - to the sorted results from the comparator here:

https://github.com/mui/mui-x/blob/d246a1fba5135a624b60cdab64776199b03eff1a/packages/x-data-grid/src/components/panel/filterPanel/GridFilterForm.tsx#L291-L306

@romgrk do you have any idea how we can support the usecase for @tomatblackcape ?

[michelengelen]

One way we could provide such a behavior if we pass the sorting mode to the comparator function instead, so an implementation could look something like this:

const comparator = (a, b, cellParams1, cellParams2, sortingMode) => {
  const modifier = sortingMode === 'asc' ? 1 : -1;
  if (a === null) {
    return modifier * 1;
  }
  if (b === null) {
    return modifier * -1;
  }
  return modifier * (a -b);
};

WDYT?

[tomatblackcape]

@michelengelen I think that method looks great! As long as the comparator function knows whether it's ASC or DESC, it can modify its behavior appropriately.

[MBilalShafi]

The proposal looks great and will allow more control to the users when it comes to sorting. One thing to consider though is we might need to break the existing API for sortComparator since handling the modifier will be offloaded to the users, and we can't just multiply -1 for descending sort internally anymore.

We could avoid the breaking change though if we go the same path as AgGrid, which just passes the 5th parameter (isDescending) as a piece of additional information that is not meant to invert the value on the user's part, i.e. grid keeps inverting the value internally, which has both downside and upside. Downside: We have to explain in docs that the 5th parameter is not for inverting purposes. Upside: Simple to use for basic use cases, the users won't have to care about the sortingMode (5th param) if they don't have an exceptional use case.

So in general we have two possible options.

1. Offload the inversion to the end users, and remove it internally. It will be a breaking change since all the existing custom comparators will have to be updated by users for it to keep working like today. Example:

   const comparator = (a, b, cellParams1, cellParams2, sortingMode) => {
     const modifier = sortingMode === 'asc' ? 1 : -1;
     if (a === null) {
       return modifier * 1;
     }
     if (b === null) {
       return modifier * -1;
     }
     return modifier * (a -b);
   };
   

2. Keep everything the same as today (keep inverting the value internally) and add a 5th parameter sortingMode for the outlying cases like this issue. Example:

   const comparator = (a, b, cellParams1, cellParams2, sortingMode) => {
     // only take into the account `sortingMode` for custom use cases like for differently sorting null values
     const modifier = sortingMode === 'asc' ? 1 : -1;
     if (a === null) {
       return modifier * 1;
     }
     if (b === null) {
       return modifier * -1;
     }
     return a -b;
   };
   

I am more inclined towards option 2 as it won't break anything for existing users, we could just add a demo utilizing the sortingMode parameter and explaining that it shouldn't be used to invert the value.

Thoughts @mui/xgrid ?

[romgrk]

I don't like option 2 because it makes the behavior more complex to understand, for the sake of preserving compatibility. I would rather introduce a new comparator API, while keeping the existing one until we possibly deprecate it.

[tomatblackcape]

How about something like this to preserve maximum compatibility:

1. Add an optional function prop to GridColDef called something like sortController. It takes a single parameter, sortingOrder, and must return a comparator function with the same signature requirements as the sortComparator prop. If provided, call this new prop every time the sorting behavior is set to 'asc' or 'desc' (not needed for null/undefined) and use the returned comparator function to perform the sort. Ignore the sortComparator prop if any - the sortController is now responsible for providing a comparator.
2. If not provided by the developer, the default behavior should remain to use the current sortComparator prop and invert its value if sortingOrder === 'desc'. This preserves the current behavior of inverting the sortComparator for 'desc' sorts and doesn't alter the behavior of existing code that uses a custom sortComparator prop.
3. If the developer wants to implement complex sorting behavior, they now have a place to define the comparator based on the sorting direction before sorting occurs. For my use case, I would either define separate 'asc' and 'desc' versions of the sortComparator and return the appropriate one inside the sortController, or I would have a parameterized sortComparator function that can handle both cases and return a curried version of the comparator for sorting.

Pros: preserves existing behavior, transparent if unused, allows conditional sorting behavior, comparator logic remains simple (no unintuitive 5th argument on the comparator itself).

Cons: adds another prop, requires careful documentation, possible to do silly things like provide a sortComparator prop alongside a sortController prop, even though the sortComparator will not be used.

Thoughts?

[cherniavskii]

@tomatblackcape Interesting idea, I like it! I would propose calling it getSortComparator though.

@romgrk @MBilalShafi What do you think?

[romgrk]

Sounds good to me.

[MBilalShafi]

I agree it's a smart idea, let's go with this. 🚀

[michelengelen]

I like it! Best of both worlds!

[github-actions[bot]]

:warning: This issue has been closed. If you have a similar problem, please open a new issue and provide details about your specific problem. If you can provide additional information related to this topic that could help future readers, please feel free to leave a comment.

How did we do @tomatblackcape? Your experience with our support team matters to us. If you have a moment, please share your thoughts through our brief survey.

[cherniavskii]

@tomatblackcape The PR was merged and will be released in the v7 release later this week. Let me know if you need this for the v6 of the data grid so I can cherry-pick it.

[tomatblackcape]

No, I can wait. Thanks so much for the quick response!

On Tue, Feb 27, 2024, 07:41 Andrew Cherniavskii @.***> wrote:

@tomatblackcape https://github.com/tomatblackcape The PR was merged and will be released in the v7 release later this week. Let me know if you need this for the v6 of the data grid so I can cherry-pick it.

— Reply to this email directly, view it on GitHub https://github.com/mui/mui-x/issues/12052#issuecomment-1966460867, or unsubscribe https://github.com/notifications/unsubscribe-auth/BDUS6TRA3HKAX3AJ5CDMU6LYVXIA3AVCNFSM6AAAAABDHJTTGOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGQ3DAOBWG4 . You are receiving this because you were mentioned.Message ID: @.***>