openfoodnetwork icon indicating copy to clipboard operation
openfoodnetwork copied to clipboard

[BUU] Don't reload whole table when cloning products

Open mariocarabotta opened this issue 1 year ago • 9 comments

When performing a clone from the actions menu, the whole table is reloaded. This can take some time, and is jarring because you can lose your position in a long list of products. The old products screen does this so we need to support it here too.

Screenshot 2023-12-28 at 14 08 16

  1. After confirming, attempt to clone the product using existing behaviour (the name has "COPY" prepended to it).
  2. If clone succeeds, insert the row(s) directly below the cloned product.
    • Try using a "slide in" animation, to avoid the jump which would cause the user to lose their place.
  3. If the action fails, show a flash message explaining why, without changing any other part of the page.

Other notes:

  • Should the pagination info be updated ("showing x of x")? For the first iteration, make no changes, this will be re-evaluated later
  • Any unsaved edits will remain on the page and not affect progress. (fixes issues when other rows have been edited as pictured)
    • If the product to be cloned has unsaved changes, should they be saved and included in the clone? We'll keep it simple and won't attempt that. As usual, unsaved changes (yellow border) are unsaved until the save button is pressed.
  • Depends on #12361
  • Very similar to #12398

mariocarabotta avatar Dec 28 '23 04:12 mariocarabotta

if we're viewing 15 per page, we'll see 14 after the action (if deleting), we'll see 16 if cloning.

estimate: 1day

mariocarabotta avatar Feb 20 '24 04:02 mariocarabotta

@mariocarabotta @RachL what is the expected behaviour for the pagination info? Asking because this is generated by the backend.

Screenshot 2024-04-15 at 19 55 05

anansilva avatar Apr 15 '24 18:04 anansilva

Hi @anansilva, apologies we have been very light on for details here. Based on previous discussions, I've written some requirements (edit: moved to description at top), but there are still some gaps (as you have identified).

TBC:

  • should the pagination info be updated?
  • should there be an animation?
  • If the product to be cloned has unsaved changes, should they be saved and included in the clone?

dacook avatar Apr 16 '24 00:04 dacook

I'll share my thoughts to start with, but Mario or Rachel please confirm what you think.

  • should the pagination info be updated?

I think it would be problematic to try and update it, because we're messing with the pagination. What if we make the text greyed out, to show that it's no longer accurate, but hasn't been regenerated yet?

  • should there be an animation?

I would suggest a simple slide in (for clone) and slide out (for delete), to avoid the jump which would cause the user to lose their place.

  • If the product to be cloned has unsaved changes, should they be saved and included in the clone?

Maybe that would be handy, but I suggest we keep it simple and don't attempt that. Unsaved changes (yellow border) are unsaved until the save button is pressed. Another option is to show a specific warning dialog ("this product has unsaved changes, do you want to submit all changes before cloning?"). But again I don't think we should attempt that. Unsaved changes are simply ignored and not cloned. They are not cleared, so can be saved after the clone is done.

dacook avatar Apr 16 '24 00:04 dacook

And finally, as mentioned in Slack, this requires a framework which isn't ready yet, so I've marked as blocked for now.

dacook avatar Apr 16 '24 00:04 dacook

For reference, the current bulk products screen does update the pagination count when deleting or cloning. See below example where there was a page of 15, and i deleted lots of products:

Screenshot 2024-04-17 at 3 30 57 pm

It still says "15 per page" which obviously isn't quite true in this case. But "Viewing 1 to 3" is mostly accurate. (It's not perfect though: if we're cloning product "Z" on the last page, a "COPY OF Z" is visible on screen underneath it. But when you reload the page you find that "COPY OF Z" is now on the first page.)

Because it's pushing the limits of Reactive Rails (with Turbo), I suggest we first try and solve it with design if possible. But clearly the precedent is there, and I think we could do it if needed.

dacook avatar Apr 17 '24 05:04 dacook

Thanks for your comments @dacook, should we split this issue into smaller ones? Here's my suggestion:

  • Issue/PR to fix the full page reload on delete
  • Issue/PR to fix the full page reload on clone
  • ... (further UI improvements on both these actions)

anansilva avatar Apr 17 '24 19:04 anansilva

Thanks Ana. I've gone ahead and split into two issues: this one for cloning, and a new one for deleting. I've also updated the description to incorporate the latest discussion. If you would like to solve this with more than one PR, that is fine.

dacook avatar Apr 18 '24 03:04 dacook

This issue is now unassigned and ready to start. Here is a work in progress that might be helpful to refer to:

  • https://github.com/openfoodfoundation/openfoodnetwork/pull/12404

dacook avatar May 20 '24 02:05 dacook