Add "Move" option for deleted items in the content tree

[kjac]

Prerequisites

• I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes: #3769

Description

This PR simply adds the "Move" action to deleted items in the content tree - see #3769 for more info.

[kjac]

Update: The extra divider above "Delete" has been removed:

[madsrasmussen]

Hi @kjac, Thanks for the PR.

I quick thought. Do we need the extra context menu item. Would it make sense to make the restore functionality "fallback" to a move if it fails?

If a restore can't be made -> show a message that we can't automatically restore the node and show a tree where the user can select where the node should go?

[kjac]

@madsrasmussen not a bad idea! My only concern is the amount of duplicate code (and/or refactoring) it'll require to implement this - perhaps it'd be better to settle for a "Click here to move it" button instead?

[kjac]

@madsrasmussen something along the lines of this:

[madsrasmussen]

To me, it feels like a workaround :)

I don't think the code duplication is that bad compared to adding the whole move dialog. If I remember correctly the restore functionality is just a move method so we need to add some extra code to handle the tree.

[kjac]

@madsrasmussen you drive a hard bargain 😄

The move dialog wouldn't be duplicated - it'd be reused. But you're right, it is a workaround. In the end the UX is much better if the move operation is embedded within the restore dialog. We'll just have to live with the duplicate code.

Here's a working demo:

[kjac]

As @madsrasmussen points out above: When a content item can't be restored automatically, it makes sense to add the "move" option directly in the "restore" dialog, alongside a message explaining what's going on.

This PR has been updated accordingly:

Note that I've kept the "move" menu item for restored items. It makes sense to have both options, just like you do e.g. in an operating system.

[poornimanayar]

Hey @kjac @madsrasmussen ,

I have been testing this PR. Quick question, given that we have the restore modal helps you manually move an item when an automatic restore cannot be achieved is it necessary to have the move option there at all? Is it not better to have a Restore which handles restore and move?

Poornima

[kjac]

@poornimanayar originally I had removed the "Move" option from the menu. But I'm not sure it makes sense to leave it out.

Consider the recycle bin on your computer:

• You can choose to restore an item in it to its original location on disk (at least you can on Windows, I can't say for sure on Mac...) - equivalent to the "Restore" menu item.
• If you don't want it in its original location, you can choose to move the item somewhere else instead, by dragging or using the folder options - equivalent to the "Move" menu item.

In other words, if people don't want the item restored to its original place (maybe they can't remember where it was in the first place?), I see no harm in letting them opt for "Move" instead.

That being said I really don't feel very strongly about it, so I'd be happy to remove "Move" (see what I did there?).

[poornimanayar]

Hello @kjac ,

Yes, that makes total sense to me :-) Different ways of seeing the same issue I think! I'll test the actual functionality quickly and comment :-)

Poornima

[poornimanayar]

I have tested this functionality and I can confirm it works. One thought I have is should the Restore Notification when using the Restore button say the same message as the restore not available message which appears in the modal? I might be overthinking it here!

Poornima

[kjac]

@poornimanayar you're not overthinking anything 😄 I assume you're referring to how both confirmation dialogs say "[source] was moved underneath [target]"?

I didn't even see that... it's simply the same message as it's always been. Well spotted!

It really would make sense to add a new message ("[source] was restored underneath [target]"), even if it's at the cost of that additional text to be translated.

I'll update the PR shortly 👍

[kjac]

@poornimanayar there we go!

[poornimanayar]

Thats a good catch , I didnt note it. What I was suggesting it may be make these two consistent , it kinda is. So I am clearly overthinking I think

[kjac]

@poornimanayar ooohhh I didn't think about that. But now that the "Move" menu item is there, I guess the message is OK?

[poornimanayar]

Yeah lets leave it at that @kjac . I shall test the latest changes you made and let you know!

Poornima

[nul800sebastiaan]

This is so smooth, love it! And thanks for all the UX feedback, really great to see the back and forth! 🏅

[kjac]

Awesome! I'll get the media section equivalent ready soon 👍

[Shazwazza]

@kjac Unfortunately there are too many changes/conficts for this to be easy for me to merge up to v8. I've just done the v7 -> v8 merge and had to ignore these specific changes. I have created a new issue to manually merge this changes to the v8 branch #3919

Since you made these changes i think this might not take you very long to apply to the v8 branch, if you have time it would be amazing if you could lend a hand, otherwise we'll have to get to re-applying this update in v8 at a later date.

[kjac]

@Shazwazza that's not a problem at all, I'll get on it soon 😄

I kinda suspected that would happen - I for one have been making changes to these dialogs in both branches. The same problem is bound to arise for the media tree equivalent if that's merged in (#3914), so don't spend time trying to merge that manually either. Just let me know and I'll reapply the changes in V8.

And for good measure: That goes for all my V7 changes. If they're giving you a headache in the merge to V8, let me know and I'll rework them in V8. No point in you guys wasting time when I have time to give 👍