Append new items to the top by default #42
Conversation
|
Thanks, I'll review this later this week given the build is failing I need to dig in further to find out the issue. I'll probably also default it to sorting to the bottom by default for backwards compatibility and performance. I may also rather than make it a global configuration option make it configurable on a per-instance basis. |
|
It's not working with many_many or with has_many relationship? I just tested it with has_many, but many_many modification is really trivial. It's ok append to the bottom by default, and the per-instance basis config it's really cool. Anyway I don't think appendig to the top will be a performance killer, since it's just a single query, not a php loop. |
|
You maybe able to narrow down the issue by looking at build 23.1 (23.2 is the same but 3.1 instead of 3.0 releases) https://travis-ci.org/UndefinedOffset/SortableGridField/builds/10973987 |
|
working with has_many relationship, at line 216 $owner it's NULL during testing, but it works fine during normal usage. Maybe it's a test issue? |
|
It's possible, I'll look deeper this weekend and see if I can identify the issue. |
|
I've was fixing the failing test but and then realized something, your code has it updating all records in the table every time the loop iterates which may not be php heavy but it would be hard on the database when the dataset is large. |
|
However though this maybe the only solution, I think for this reason alone I'll default it to off by default and caution in the notes about large datasets. Also this will cause sql errors when there is no parent relationship like in the case of having the list be sorted on model admin which is why the test was failing. |
|
Sorry for the delay been really busy, I've created a new branch https://github.com/UndefinedOffset/SortableGridField/tree/experimental with this change merged in with a few modifications. Rather than make a default on or off for top sorting, it is a configuration method $inst->setAppendToTop(true), as well I've accounted for model admin. Do you mind testing in your env see if this solution works for you? If it does I'll merge the branch back into master and close this pull. |
|
It does work on my test environment. Good joob 👌 |
|
cool i'll merge it to master hopefully today |
No description provided.