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

Added sortOrder option (deprecating sortDirection on columns object) #1310

Merged
merged 3 commits into from
Jun 5, 2020

Conversation

patorjk
Copy link
Collaborator

@patorjk patorjk commented Jun 4, 2020

In the current API, the table's sort direction is held in one of the column options objects. This has the following problems:

  • It leads to ambiguity if multiple columns have this field set.
  • It's makes it hard to implement multiple level sorting (where a table is first sorted by 1 column, and then for values that are the same a second column is used to sort). You can see an example of this in jQuery Datatables - click a column header, and then shift+click another header. The second column is used to break ties.
  • It adds unnecessary complexity to users who use the serverSide option, since a sort value is typically passed to the server separately, and in the current implementation it burred in one of the column's options.

This PR removes sortDirection from the column's object, and creates a "sortOrder" property on the options, examplet:

var options = {
    sortOrder: {
        name: ${selectedColumn},
        direction: ${direction}
    }
};

I may change columnName to "name" and sortDirection to "direction".

@coveralls
Copy link

coveralls commented Jun 4, 2020

Coverage Status

Coverage increased (+0.6%) to 75.972% when pulling 2f087d2 on v3_sortOrder into b8eec4c on v3.

@wdh2100
Copy link
Collaborator

wdh2100 commented Jun 4, 2020

it seems to be a very good change.

sortDirection string   Set default sort order enum('asc', 'desc', 'none') (null option has been deprecated, use 'none' instead)
that's confusing in my opinion

I may change columnName to "name" and sortDirection to "direction".

var options = {
    sortDirection: {
        name: ${selectedColumn},
        direction: ${direction}
    }
};

me, too 👍

serverSide should change the option, right?

Oh 😃 . already applied to the sample.

https://github.com/gregnb/mui-datatables/pull/1310/files#diff-f7e1f12cc2cd7d7d0b7e0aba3823d92bR88-R91

but I think we should consider backward compatibility.

https://github.com/gregnb/mui-datatables/pull/1310/files#diff-4f4aff53b74d2c551e37786d32b82e3aR486

Is this enough?

@patorjk
Copy link
Collaborator Author

patorjk commented Jun 4, 2020

@wdh2100 I think you're right, backward compatibility for something like this is a good idea. I've updated the PR to account for "sortDirection" if no sortOrder value is present. I also updated the field names.

@patorjk patorjk added the v3 Denotes that this issue is required for v3 release of this library label Jun 4, 2020
@patorjk patorjk merged commit 4b8012a into v3 Jun 5, 2020
@patorjk patorjk mentioned this pull request Jun 5, 2020
@mxmlnglt
Copy link
Contributor

@patorjk you said earlier "It's makes it hard to implement multiple level sorting": can you confirm this is not yet implemented?

@patorjk
Copy link
Collaborator Author

patorjk commented Jun 29, 2020

Correct, it's not yet implemented.

@broncha
Copy link

broncha commented Jan 19, 2021

Didn't this disable sort by multiple columns? Now you can only sort by one column

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 Denotes that this issue is required for v3 release of this library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants