-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Update orderBy #269
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
Update orderBy #269
Conversation
- `{String} [order] - default: 1` | ||
|
||
- **Usage:** | ||
|
||
Return a sorted version of the source Array. The `sortKey` is the key to use for the sorting. The optional `order` argument specifies whether the result should be in ascending (`order >= 0`) or descending (`order < 0`) order. | ||
Return a sorted version of the source Array. `sortKeys` is an Array with the keys to use for the sorting. You can pass a String if you only need to sort on one key. You can also pass a Function if you want to use your own sorting strategy instead. The optional `order` argument specifies whether the result should be in ascending (`order >= 0`) or descending (`order < 0`) order. |
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.
Some suggestions:
- ... Array. The
sortKeys
is ... - You can pass a String if you need to sort only one key.
Added some suggestions before merge this. |
ping @ludo237 |
For me is fine @yyx990803 what do you think? |
|
||
``` html | ||
<ul> | ||
<li v-for="user in users | orderBy ['lastName', 'firstName']"> |
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.
Just so you know you can't use inline arrays like this as filter arguments. It breaks the parsing currently.
I forgot to mention that orderBy
should support inline multiple sortKeys like
<li v-for="user in users | orderBy 'lastName' 'firstName'"></li>
One of the reason being that the above inline array wouldn't work.
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.
So turns out this makes it more complex than I thought - I'll take a stab at rewriting it to see if it can be made simpler.
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.
vuejs/vue@ef55582 (sorry I had to change a lot of your code)
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.
Okay, it's funny because I did it that way at the beginning 😆 but then I realised that in some cases you may want to apply a variable amount of parameters defined by some property
Let me know if you want me to take care of it
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.
Sorry, didn't see the last comment.
That's fine, thanks for posting the commit 😉
Should I add an example about using an dynamic Array to sort? |
@@ -2169,7 +2169,7 @@ type: api | |||
|
|||
``` html | |||
<ul> | |||
<li v-for="user in users | orderBy ['lastName', 'firstName']"> | |||
<li v-for="user in users | orderBy 'lastName', 'firstName'"> |
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.
no need for the comma here
@posva I think just adding a note that you can use an Array is fine. Also GitHub now supports auto-squashing for merges, so no need to worry about commit numbers anymore :D |
@yyx990803 Cool 😄 |
This refers to the vuejs/vue#2591 and vuejs/vue#2562 features.
Please tell me if I did the PR on the wrong branch so I can create a new one 😄