Naxon / nova-field-sortable

Adds sorting functionality to the Laravel Nova's index resource
MIT License
60 stars 23 forks source link

Not sorting as aspected in relation index #20

Closed jakobfdev closed 3 years ago

jakobfdev commented 5 years ago

Hi Naxon,

as far as I can see, the moveUp moveDown buttons in the list don't work as expected, when used in a detail view, where the sortable-model is a relation and therefore shown in a list below the detail data.

For example: Album HasMany Images

In the Images resource index view (all Images in the DB are shown) the ordering works fine.

In the Album resource detail view (all of the particular Album's related Images are shown as list) the ordering works unexpected.

Actual behavior: The related image moves up in the full list of Images. Expected behavior: The related image moves up in the related list of images.

This is reproducible, if the images table looks like this:

id | sortorder | album_id
1  |         1 |        1
2  |         2 |        2
3  |         3 |        1

After moving image-id3 up, it changes place with image-id2, therefore doesn't move in the scope of album-id1.

Since all relations are shown in this detail view and any other scope (din't test this case), this unexpected behavior can't be hidden and will lead to confusion.

I hope you will acknowledge this issue. I like the simplicity of using your field with the spatie trait. Only this bug confuses the users.

Thanks and keep on the good work!

jakobfdev commented 5 years ago

PS: MyModel::swapOrder($myModel, $anotherModel); from the Spatie trait might be a suitable and quick solution if access to the predecessor in the related list is given in the vue component.

jeffreymoen commented 5 years ago

I'm running into a very similar issue. I have a model called Categories, which contains fields like id, parent_id, & display_order. With the intent being that all records with the same parent_id are ordered together. However, the package seems to work on the entire model and not the subset of data that is being displayed in Nova, which leads to the changing the display_order for the entire model and not the subset of data that is being displayed (e.g. records with the same parent_id). I've looked at the documentation for both this package and the original Spatie/eloquent-sortable package and I am not quite sure how to handle this use case.

Not necessarily a package problem, but I am noting this since jBOKA is running into a very similar issue.

jakobfdev commented 5 years ago

Your trying to handle an ordered tree, this is not the intend of the package. But your problem has the same origin.

Yelles commented 5 years ago

I'm running into a very similar issue. I have a model called Categories, which contains fields like id, parent_id, & display_order. With the intent being that all records with the same parent_id are ordered together. However, the package seems to work on the entire model and not the subset of data that is being displayed in Nova, which leads to the changing the display_order for the entire model and not the subset of data that is being displayed (e.g. records with the same parent_id). I've looked at the documentation for both this package and the original Spatie/eloquent-sortable package and I am not quite sure how to handle this use case.

Not necessarily a package problem, but I am noting this since jBOKA is running into a very similar issue.

Did you find a solution for your issue ? i'm looking for the same :)

jakobfdev commented 5 years ago

Did not have the time to look into this

jakobfdev commented 5 years ago

After digging into the nature of this being a field, currently there is no way I can see, to give the moveUp & moveDown call more context in regard to the scoped lists as in relation index views.

Also the underlying model's sortable interface (spatie/eloquent-sortable package) doesn't provide a moveBefore($model) or moveAfter($model), which would be necessary. Even if the spatie package would be extended, this package would still need some work (if even possible in terms of contact. The up/down buttons just don't work within list context as one might wish, but in the whole table of the model.

This cries for another approach for sorting in general.

For flat model relations it is still perfectly suitable!

EDIT: Since the field approach doesn't allow any more context regarding a scoped list index, this is probably gonna stay like it is. I think the issue can be closed.

@Naxon The limitation e.g. for relation-indexes should be mentioned in the readme, so people don't have to run into this first.

Still thanks for this clear and simple approach.

mizmiz commented 5 years ago

The workaround I came up with is to remove the sorting column in the related list:

Sortable::make('Sortieren', 'id')
                ->onlyOnIndex()
                ->canSee(function(Request $request) {
                    return empty($request->get('viaResource'));
                }),
Naxon commented 3 years ago

As some said, this package is not intended to support multi-level sorting.

As I don't currently need this functionality for my own use, I can't develop this right now. If anyone wants to add this to v2 - please PR.

Thanks :)