Add drag and drop prioritization to K2 #252
Conversation
|
@neil-marcellini Noob question. How do I package and install this in my browser for testing? I have tried downloading the source code as zip from your branch, and uploading this as unpacked but get a manifest error.
|
Please git clone the repo and follow the instructions in the README. Let me know if you have other questions. |
|
Also requesting a review from Tim because I think he's done a lot of work on K2 |
|
Able to replicate testing without issues. Including positions after page refresh. K2.testing.mp4 |
|
Oh, Tim is OOO for a week so I'll ask someone else. |
There was a problem hiding this comment.
Neat! code looks good - only issue - react-beautiful-dnd is dying at the end of April. We're replacing it with dnd-kit in App, we might want to do this here as well? Expensify/App#59231
Let me know what you think, it might not be worth the extra work but would be more future proof.
|
Oh wow great to know, I'm glad Pullerbear chose you to review. Yeah sure I can try switching it to dnd-kit. |
|
Wow updating to that new library was kind of a pain, since it seems to be a bit more complex, but it's working well now! Please review again. |
|
Thanks for the review @tgolen. Updated. |
| // Update local order immediately for UI feedback | ||
| const newOrder = arrayMove(_.pluck(filteredData, 'id'), oldIndex, newIndex); | ||
| setLocalOrder(newOrder); |
There was a problem hiding this comment.
Can you help me understand why the local order is being used, rather than keeping everything in Onyx and having this component subscribe to the Onyx priorities? That would still give the user immediate feedback, wouldn't it?
There was a problem hiding this comment.
Sure. Everything is kept in Onyx and the component subscribes through useOnyx to the priorities. Originally I did not use the local order state and used Onyx only. It worked fine with the react-beautiful-dnd library. Later, I switched to dnd-kit, since the other library is going to be deprecated soon. After that there was an issue with the UI where the dragged item would briefly jump back to it's original position, then jump to where it was dropped. I believe that's because Onyx.set is not synchronous, and that's further confirmed by the fact that using and setting this localOrder state fixes the problem.
I definitely don't love it since it's pretty complicated to keep the state in sync. However, it seems necessary to make this work properly. I can record a video of the bug if you like or you can checkout a commit before I added the local order and view it yourself.
Are you ok with leaving this in now? Should I clarify any comments in the code? Is there something else you would recommend?
There was a problem hiding this comment.
OK, thanks for explaining that a little. Did you happen to look into https://github.com/react-dnd/react-dnd/ at all? Maybe it's worth giving it a quick look to see if we don't need to do this local state thing.
There was a problem hiding this comment.
Yeah I guess I can try that out
There was a problem hiding this comment.
I tried out react-dnd briefly, using Onyx only, and it had similar and even worse issues. I believe this flashing is due to repeated updates to the component when the hovering and the async nature of them.
If you're cool with it I think we should keep the current library and keep using local state for synchronous updates.
2025-04-22_20-09-53.mp4
There was a problem hiding this comment.
OK, sounds good. Thanks for looking into it.
Yeah I was thinking that too! I wanted to keep the initial version focused but that would certainly make it feel more complete. It would also be nice to be able to view other people's priorities when viewing their dashboards, but I'm not sure where we would store the information tied to a user's account. |
Yeah, there is no central place to store this. There used to be some code in here which kept a shared "database" in a GH Gist. I put database in quotes because it was just a Gist with a JSON file that could be written to and read from. The file was here but removed at some point because we stopped using it. |
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/480259
Tests
2025-04-18_13-01-17.mp4