-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Change the definition of sort order to ASC|DESC #8466
Conversation
44acab1
to
af42032
Compare
af42032
to
895cb8d
Compare
Thanks for your contribution. You are correct, there are places in the code where we actually expect the value to be one of these, but since it's not currently enforced, I'm afraid that restricting the allowed types may break things. |
Got it! I totally understand that there is a risk regarding the type definition change. I will leave this PR here and it is totally upon your decision on what to do with the PR. |
Hi, and sorry it took us so long to look at your PR. We're ready to move forward. I've merged Also, as there may be an impact on some codebases, I'd prefer that we publish this change in a minor release. Would you mind rebasing on |
|
ec5543b
to
b185871
Compare
b185871
to
63fd92b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, thanks!
not to be a buzz kill but this is a pain for ie. ra-data-hasura where there are more sorting options i'd revise the reasoning behind this change and revert the |
Thanks for your feedback. Compound sort order values aren't compatible with our datagrid headers, which expect a single order. Can you explain how you manage datagrids in this case? |
Currently
order
inSortPayload
type definition is a general string, but it shall beASC
orDESC
in accordance with the docs and sort clicks inDatagrid
This PR changes the definition oforder
toASC|DESC
I also changed some
desc
in spec or demo files toDESC
, because, if we useDatagrid
from the react-admin, then our order would be predefinedDESC
orASC
, not the small caseddesc
orasc
.