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

Rows that have no value are sorted on top in ascending order #1511

Open
kaulfield23 opened this issue Sep 7, 2023 · 2 comments
Open

Rows that have no value are sorted on top in ascending order #1511

kaulfield23 opened this issue Sep 7, 2023 · 2 comments
Labels
🐜 bug Something isn't working 🛑 potentially blocked Potentially blocked by prerequisites (double check or ask someone, as it may have changed)

Comments

@kaulfield23
Copy link
Contributor

kaulfield23 commented Sep 7, 2023

Please be as thorough as possible, but if you can't answer everything, please just submit what you have! Thank you!

Description

When sorting happens in view column, normally it is expected to see values at the top of the list in ascending or descending order. However, empty cells are prioritized over cells that have values in view. Therefore, it can be inconvenient to scroll down to see the values in ascending order.

Steps to reproduce

  1. Go to view
  2. Add column Notes (or any other columns)
  3. Give values to each cells. Make sure to leave some cells empty.
  4. Sort the Notes column.

Expected Behaviour

The sorting should show values first before empty cells. So the order should be "values - null(empty cells)" in ascending / descending order in all type of columns.

Actual Behaviour

It has this following order.

  • In ascending order : "null - values"
  • In descending order : "values - null"

Screenshots (if you have any)

Ascending order
image

Descending order
image

@kaulfield23 kaulfield23 added 🐜 bug Something isn't working 🎨 needs-design labels Sep 7, 2023
@kaulfield23 kaulfield23 changed the title Rows that has no value sorted on top in ascending order in view Rows that have no value are sorted on top in ascending order Sep 7, 2023
@Bagera
Copy link
Contributor

Bagera commented Jun 13, 2024

I think null/empty cells should be disregarded in the ordering and added at the bottom no matter the direction. No more design is needed in my opinion.

@henrycatalinismith
Copy link
Collaborator

There's an example in the Data Grid docs that demonstrates how to achieve this exact sorting behavior.

The Data Grid considers the sortComparator function symmetric, automatically reversing the return value for descending sorting by multiplying it by -1.

While this is sufficient for most use cases, it is possible to define an asymmetric comparator using the getSortComparator function – it receives the sorting direction as an argument and returns a comparator function.

In the demo below, the getSortComparator function is used in the "Quantity" column to keep the null values at the bottom when sorting is applied (regardless of the sorting direction):

https://mui.com/x/react-data-grid/sorting/#asymmetric-comparator

The implementation then showcases a getSortComparator prop being added to the GridColDef.

getSortComparator: (sortDirection) => {
  const modifier = sortDirection === 'desc' ? -1 : 1;
  return (value1, value2, cellParams1, cellParams2) => {
    if (value1 === null) {
      return 1;
    }
    if (value2 === null) {
      return -1;
    }
    return (
      modifier *
      gridStringOrNumberComparator(value1, value2, cellParams1, cellParams2)
    );
  };
},

I've looked at adding this custom comparator. It will be relatively straightforward to add this to useConfigurableDataGridColumns.

The blocker here is that we're using a @mui/x-data-grid-pro version from 2023 (6.14.0), and the PR adding getSortComparator happened in early 2024. It looks like we need to upgrade this dependency to at least 7.0.0 in order to use this.

The notes on the migration from v6 to v7 are interesting enough that it probably deserves to be an issue of its own. I found the PR where the upgrade to v6 happened and it looks like that one took a few weeks to tame!

@henrycatalinismith henrycatalinismith added the 🛑 potentially blocked Potentially blocked by prerequisites (double check or ask someone, as it may have changed) label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐜 bug Something isn't working 🛑 potentially blocked Potentially blocked by prerequisites (double check or ask someone, as it may have changed)
Projects
None yet
Development

No branches or pull requests

3 participants