DevExpress / devextreme-reactive

Business React components for Bootstrap and Material-UI
https://devexpress.github.io/devextreme-reactive/
Other
2.07k stars 376 forks source link

Sorting Column Extensions not removing previous sort column #3647

Closed mmatuk closed 1 year ago

mmatuk commented 1 year ago

Is there an existing issue for this?

I'm using ...

React Grid

Current Behaviour

Currently, when you pass columnExtensions to the SortingState plugin, you must supply sortingEnabled for every column that you passed. If you forget to pass sortingEnabled: true, even though sorting is enabled for all columns by default, sorting breaks and does not remove the sorting of the previously selected column.

This is the column extension: [ { columnName: "name" }, { columnName: "gender", sortingEnabled: false }, { columnName: "city" }, { columnName: "car" } ]

This is what happens when you click on multiple columns to change sorting: image

This looks to be caused by a change to the base getPersistentSortedColumns function. Instead of looking for explicit false, it now just looks for falsely values which means sortingEnabled == undefined now triggers this code.

This file packages/dx-grid-core/src/plugins/sorting-state/helpers.ts from this commit https://github.com/DevExpress/devextreme-reactive/commit/629e9057c0562c7d11fc5400f725555ab327fc74 looks to be the cause of the issue.

Expected Behaviour

Prior to version 3.0.0, you could pass something like this: [ { columnName: "name" }, { columnName: "gender", sortingEnabled: false }, { columnName: "city" }, { columnName: "car" } ]

and the sorting would handle it correctly and default to sortingEnabled == true for any column that did not have a sortingEnabled config set in the columnExtensions.

I should be able to pass columnExtensions and not have to provide a config for every column. The function getPersistentSortedColumns should respect the columnSortingEnabled prop that is passed to the main SortingState plugin. Currently is does not.

Steps to Reproduce

See this example: https://codesandbox.io/s/grid-table-sorting-2jok85?file=/demo.js:708-848

  1. Click on different column headers to change the sorting and notice it does not remove the previous sorting and that the sorting does not update correctly either.
  2. You can also, change the versions of gird back to something before 3.0.0 and redo step 1 to see how it use to work.

Environment

Madobyte commented 1 year ago

Hello @mmatuk,

We refactored our code to follow our documentation. The sortingEnabled property should always be specified and should not be null or undefined according to the following help topic: SortingState.ColumnExtension. Could you clarify if it's critical for your project to use the old logic?

mmatuk commented 1 year ago

@Madobyte

Thank you getting back to me. We have a wrapper component around the SortingState that always injects a sortingEnabled value that matches the default sortingEnabled flag into the passed columnExtenstions array. This fixes the issue for us. In my opinion, though, it makes more sense to not require sortingEnabled flag for every column if you only want it enabled for one or two columns. The individual column config should allow overriding the default sortingEnabled flag and if not defined then it should fallback to the default. The current behavior does not do this. If you forget to add a sortingEnabled flag, even if it matches the default, you get some unexpected behavior as seen in the example above.

Your current documentation also does not indicate that if you add the sortingEnabled flag for even one column, then you must add it for every column even if it matches the columnSortingEnabled flag.

Madobyte commented 1 year ago

Hello @mmatuk,

In our documentation, we add ? after property names that can be null/undefined. You can see examples here: https://devexpress.github.io/devextreme-reactive/react/grid/docs/reference/sorting-state/#properties. The sortingEnabled property in SortingState.ColumnExtension does not have ?, which means that it should be explicitly defined.

In any case, we appreciate your feedback and we'll consider adding a default value to the sortingEnabled property.