box-ui-elements
box-ui-elements copied to clipboard
box
fix(contentpicker): correctly clear old item when maxSelectable=1 and parent reloaded
This change fixes #1705, which was preventing the prior selection from being properly cleared when the content picker was in single file selection mode and the folder enclosing the previously selected item had been reloaded before changing the selection.
Problem
Because the content picker relied on a mutating a reference to the previously selected item, when the items were reloaded from the server, that reference was no longer valid and the item within currentCollection was not updated.
Solution
Scan the currentCollection for an item that matches the prior selection based on the item's id. If found, remove the selected keyword from that item directly.
Hi @cgrdavies, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla
Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks!
BTW there is a possibility this issue applies to multi-selection. Trying to see when it broke, likely when we switched to paging.
In order to properly fix this, and frankly make it less confusing, we may have to get rid of the reference mutations and keep selected isolated from currentCollection.items. I am trying a few things, will report back.
Roughly something along these lines might be better https://github.com/box/box-ui-elements/pull/1709/files. We still can use a map to prevent multiple find()s so that its a O(1) lookup or we can just keep the find()s as in practice selected will always be a small-ish array. Do you want to update your PR following this appraoch?
Roughly something along these lines might be better https://github.com/box/box-ui-elements/pull/1709/files. We still can use a map to prevent multiple find()s so that its a O(1) lookup or we can just keep the find()s as in practice
selectedwill always be a small-ish array. Do you want to update your PR following this appraoch?
@priyajeet Thanks for taking a look at this! I agree that removing the reference mutations is a better approach, but I was originally hesitant to do so given the magnitude of the change that would be required. With your outline though I'm happy to update this PR.
BTW, if you dont have time to make more changes, we can take over the PR...
@priyajeet should be all set from a code perspective. I changed updateItemInCollection to replace the matching item in the selected array.
I do have a failing test though that seems unrelated, and I can't immediately figure out what's going on. Any chance someone could take a look at that? I don't have enough overall context to be able to fix it expediently.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
Hey @cgrdavies, sorry we've taken so long to merge this PR. I'm looking into the failing tests right now. Is it okay with you if I add commits to your PR? Also, could you sign the new CLA? Thanks!
