Skip to content

Issues/1246 #1323

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

Merged
merged 2 commits into from
Aug 22, 2017
Merged

Issues/1246 #1323

merged 2 commits into from
Aug 22, 2017

Conversation

vincentchalamon
Copy link
Contributor

@vincentchalamon vincentchalamon commented Aug 22, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1246, #1321 (comment)
License MIT
Doc PR api-platform/docs#235

@@ -24,7 +23,7 @@
* @author Vincent Chalamon <[email protected]>
*
* @ApiResource(attributes={
* "order"={"name", "DESC"}
* "order"={"bar": "ASC", "name": "DESC"}
Copy link
Member

Choose a reason for hiding this comment

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

Just some nitpicking 😬: it would be great to also allow "order"={"bar", "name"} (that would be equivalent to "order"={"bar": "ASC", "name": "ASC"}. It's just some syntactic sugar but it improves the overall DX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dunglas Could it also be interesting to order by association property, like dummy.name?

Copy link
Member

Choose a reason for hiding this comment

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

Definitely. In another PR?

@dunglas dunglas merged commit fc822b3 into api-platform:master Aug 22, 2017
@dunglas
Copy link
Member

dunglas commented Aug 22, 2017

Thanks @vincentchalamon

@vincentchalamon
Copy link
Contributor Author

@dunglas Still a comment pending… You merged too fast !

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
* Fix api-platform#1246 after merge

* Fix api-platform#1246: add default order, allow multiple orders with optional direction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants