-
-
Notifications
You must be signed in to change notification settings - Fork 805
Sort dnd #856
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
base: master
Are you sure you want to change the base?
Sort dnd #856
Conversation
| const {active, over} = event; | ||
|
|
||
| if (over && active.id !== over.id) { | ||
| setLocalApps((items) => { |
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.
I don't think we need to expose both the sort order integer and the new api with the list of application ids. One should suffice.
If we choose the new reorder api we can hide the sort index in the application model which prevents breaking api changes (new field in the PUT /application/{id} endpoint). This would also expose less internals in the public api.
I think there is an argument for the sort index on the application. Fractional Indexing could be used that has some feature for collaborative editing (which we don't need) but this also allows us to move an application to a specific position without having to update the sort order index of all applications. The fractional index would be a text field that's sortable and the client would be able to update the fractionalIndex in the PUT /application/{id} endpoint with something like https://www.npmjs.com/package/fractional-indexing.
I don't think I've a clear best solution here. I'm slightly leaning towards the new api as it seems less complex but the fractional index solution only has to update a single application on reordering and doesn't need another endpoint.
@eternal-flame-AD do you have an opinion on this?
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.
Shoot sorry, this drop out of my radar through a mixture of my own fault and a slightly confusing title ..
Fractional indexing should be fine, I don't really think adding an optional field with a sensible default counts as breaking change.
However my understanding is fractional indexing is not concurrent safe? Let's say you have app with ID 1,2 and 3 and you want to put 3 between 1 and 2. By the time you retrieved the fractional keys for 1 and 2 and generated a new key for 3, you can't say put "newKey" between 1 and 3 (as the keys for them have already changed). I think this might be the "collaborative editing" part.
Solutions can include:
- Don't worry about it.
- Read-Check-Update: you don't say "insert newKey between 1 and 2", you say "insert newKey between oldKeyFor1 and oldKeyFor2" and loop until the server successfully made one single atomic transaction.
- Just design a separate API specifically for "put A between B and C, by ID" and write all the indexing logic on the server side
|
is there an update on this @eternal-flame-AD, @jmattheis and @xavier-GitHub76 ? |
I merged the changes made by
jberlyn (from pull request #804 to the updated master branch)
I then added drag 'n' drop sorting support.
dnd_sortorder.webm