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

[DataGrid] Allow to programmatically sort unsortable columns #11512

Merged
merged 10 commits into from
Jan 13, 2024

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Dec 24, 2023

Handles colDef.sortable part of #10552

ToDos:

  • Update docs
  • Update migration guide

Changelog

Breaking changes

Preview: https://deploy-preview-11512--material-ui-x.netlify.app/

@MBilalShafi MBilalShafi added breaking change 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 v7.x labels Dec 24, 2023
expect(getColumnValues(0)).to.deep.equal(['10', '0', '5']);
});

it('should allow clearing the current sorting using `sortColumn` idempotently', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug in sortColumn, i.e. on calling sortColumn('field', null), instead of removing the current sort it circulated between ascending and descending sort.

CSB reproduction, keep pressing None button:
Before: https://codesandbox.io/p/sandbox/holy-field-kf2jyd?file=%2Fsrc%2FDemo.tsx%3A63%2C19
After: https://codesandbox.io/p/sandbox/jovial-bash-9kg32f?file=%2Fpackage.json%3A6%2C88

@MBilalShafi MBilalShafi changed the title [DataGrid] Allow api method sortColumn to work for unsortable columns [DataGrid] Allow to programmatially set sort rules for unsortable columns Jan 1, 2024
@@ -71,6 +71,14 @@ In the following demo, the user cannot use the _rating_ column as a sorting rule

{{"demo": "DisableSortingGrid.js", "bg": "inline", "defaultCodeOpen": false}}

### Read-only sorting
Copy link
Member Author

@MBilalShafi MBilalShafi Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't think of a more appropriate name for this, technically it is read-only but from the perspective of the grid's default UI, it could be implemented on the developer's side to be not-read-only, like in the example below. Read-only sorting could be sold as a use-case, I can imagine it being useful in certain circumstances.
Feel free to suggest more names though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I also don't like the name, I think it makes it sound more complicated than it is. What we want to say is that the sortable flag controls whether or not the sorting is enabled for the end-user from the UI. I think I would integrate that information to the paragraph above, in Disable the sorting, and maybe merge both examples together.

Copy link
Member Author

@MBilalShafi MBilalShafi Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of sorting only, I couldn't agree more, but after working on #11538 it seemed to be more than just disabling a column filter. Considering this example https://codesandbox.io/p/sandbox/bold-platform-9jg53c?file=%2Fsrc%2FDemo.tsx%3A41%2C9

  1. The filters will be there in the filter panel with a slightly different experience (disabled/read only) so technically it will impact the default Grid UI
  2. It could be a genuine use-case for some users, imagine some applied filters some of which are not allowed to be changed by a specific user type.
  3. Putting it under a separate heading may improve the discoverability and present this as a concept.

So keeping in mind the behavior for filtering, I thought it'd be better to present this as a concept (read-only object) for other flags too, that way it will be more:

I might be too pushy here though, so would appreciate others' thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok your reasons make sense, let's go with that.

Copy link
Contributor

@romgrk romgrk Jan 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read again and I still feel like it should be in the same paragraph as Disable the sorting. Because both paragraph describe the same thing: the behavior of the .sortable flag. They both talk about what happens when sortable: false. Splitting it into a different paragraph makes it somewhat easier to discover that case if one is looking very specifically for that, but it splits the information about the behavior of the flag into two paragraphs/sections. Besides, if someone is looking for that use-case, are they more likely to search the docs for "disable sorting" or for "read-only sorting"? Imho, first one is more likely.

It's a soft disagree though, so if that doesn't convince you let's leave it as it is.

Copy link
Member Author

@MBilalShafi MBilalShafi Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, now there are two options here.

  1. Make it a sub-section or part of Disable the ${featureName} > For columns.
  2. Keep it a separate section, put it on the same nested level as Disable the ${featureName} but name it more generalized (like @cherniavskii suggested) for better discoverability.

I also noticed we miss a global disabling prop (like disableColumnSort) for sorting like we have for every other feature (e.g. disableColumnFilter, disableColumnMenu, disableVIrtualization, ...), that is why we don't have a document structure like this (For all columns and For some columns nested levels) in sorting.

I think we should add this flag to the sorting and if it is missing elsewhere (I have to check) and then make documentation for every feature like so:

### Disable ${featureName}

#### For all columns

content...

#### For some columns

content...

#### `${featureName}ing non-${featureName}able columns programmatically`

content...

This way it will be:

  1. A sub-section of the Disable section (@romgrk's point)
  2. A separate section for this specific option presented as a concept (better for SEO and discoverability)
  3. It will be consistent and aligned in all the objects, thus could be mentioned as a central behavior in docs pages like FAQs.

Any objections?

Copy link
Member Author

@MBilalShafi MBilalShafi Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another related question, which applies to all the features, should we allow programmatically sorting some columns if the sorting feature is disabled altogether (i.e using the proposed prop disableColumnSorting), I don't think so, wdyt?
@romgrk @cherniavskii

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection to the first comment.

For the behavior of the disableColumnXxxx flags, I'm not sure what's their use-case. Without context I'd argue that they should control the UI and not disable programmatic actions, just like the column-local flags. But depending on their historic use I could change my opinion. As long as they're all consistent I'm happy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the behavior of the disableColumnXxxx flags, I'm not sure what's their use-case.

As how I understand these flags, these refer to disabling the entire feature. Like how the term implies, for instance disableColumnFiltering, the most accurate understanding I can derive from this is turning the entire filtering feature off. That is the reason I suggested not to consider supporting programmatic actions for such features.

Copy link
Member Author

@MBilalShafi MBilalShafi Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I was a bit mistaken on this, disableColumnFilter does allow to control, initialize or use API method to add filters programmatically. It only disables stuff on the UI.
Kept the same behavior on this one too.

@MBilalShafi MBilalShafi marked this pull request as ready for review January 2, 2024 06:40
@MBilalShafi MBilalShafi changed the title [DataGrid] Allow to programmatially set sort rules for unsortable columns [DataGrid] Allow to programmatially sort unsortable columns Jan 2, 2024
@MBilalShafi MBilalShafi changed the title [DataGrid] Allow to programmatially sort unsortable columns [DataGrid] Allow to programmatically sort unsortable columns Jan 2, 2024
@MBilalShafi
Copy link
Member Author

New updates:

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2024
Copy link

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 11, 2024
@MBilalShafi MBilalShafi merged commit 721fe5c into mui:next Jan 13, 2024
17 checks passed
@MBilalShafi MBilalShafi deleted the fix-10552/sortable branch January 13, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 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 v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants