OrderList: keep correct order when moving multiple to top or bottom #4701
Conversation
…r bottom When multiple items are selected and moved to the top or bottom of an OrderList at the same time, they are moved one by one in a loop until the entire selection has been moved. To make sure that the item at the top of the selection (the selected item that is highest up in the current order) will still be at the top after the selection, we should move items in the right order: move the top item last when inserting at the top, and first when inserting at the bottom. Currently, this order is exactly reversed, meaning the selected items are reversed in those cases. This commit reverses both loop orders to fix this problem
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
I still think there is a bug here. When I test your code if I CTRL+CLICK 2 items and try and move them up and down I can't always it seems to depend on the order I click them? |
|
@melloware thanks for testing the PR! Could you describe what you did in more detail and what went wrong? Played around with the version from my PR a bit and I cannot reproduce this, I tried clicking and moving in various orders: Screencast.from.2023-10-26.21-48-06.webmDid your problem occur when moving things up and down one step or all the way to the top/bottom? I only changed the code for moving moving to top or bottom, the logic for selecting items and moving them one step should be exactly the same as in the original version. |
|
OK if I click non-contiguous cells it works but check out my video if you select the First two records contiguous and press down it won't go down.
|
|
Thanks again! Something is clearly still going wrong there. I still cannot reproduce this problem, in my case moving two items down from the top works without any problem no matter in which order I move/select/deselect them: Screencast.from.2023-10-26.23-43-19.webmDo you think this problem was introduced by my PR, or that this is an unrelated problem? I don't think it is likely my changes introduced this problem, since they don't affect the selection logic or the "one place down" button, but to be sure it would be good to exclude the following possibilities:
|
|
Actually I forgot to mention I am porting your fix to PrimeReact since it has the same code. But clearly something in PR is different that i am seeing this behavior. Let me do some more comparison of PV and PR code to see what is different. |
|
Since the issue @melloware encountered only applies to PrimeReact and is unrelated to this PR, and my fix has already been apporved integrated into the React version (primefaces/primereact#5178), can we merge this here as well? I don't know how to move this forward, is there anything else I can do? I already created an issue: #4700 As mentioned, the same issue also applies to the Vue 3 version. I think the fix could be applied as is here: https://github.com/primefaces/primevue/blob/master/components/lib/orderlist/OrderList.vue#L381 Would it help if I change the PR to target master first, and then backport it to the |
|
You can submit pR for both branches. PrimeTek will eventually review! |
|
@jeroenmuller If you're interested in adding this same solution to PrimeVue version 3, we'll gladly merge it. Thanks for your contribution! |
This PR fixes a bug in the OrderList component when moving multiple items to the top or bottom.
Fixes #4700
When multiple items are selected and moved to the top or bottom of an OrderList at the same time, they are moved one by one in a loop until the entire selection has been moved.
To make sure that the item at the top of the selection (the selected item that is highest up in the current order) will still be at the top after the selection, we should move items in the right order: move the top item last when inserting at the top, and first when inserting at the bottom. Currently, this order is exactly reversed, meaning the selected items are reversed in those cases.
This commit reverses both loop orders to fix this problem