Add page trash frontend #709

mejo- · 2023-05-30T14:46:52Z

📝 Summary

This PR depends on #700.

Resolves: Allow restoring a deleted page #47

🖼️ Screenshots and screencasts

Delete from page list	Restore from page trash	Mobile

Screencast
recording2.webm

🚧 TODO

Feedback from design team
In mobile view, delete button is not sticky to the buttom. why?

🏁 Checklist

Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
Sign-off message is added to all commits
Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
Documentation (README or documentation) has been updated or is not required

cypress · 2023-05-30T15:22:39Z

15 failed tests on run #825 ↗︎

15	57	0	0	0

Details:

Add page trash frontend
Project: Collectives	Commit: `d7c6cb8f0d`
Status: Failed	Duration: 23:00 💡
Started: Jun 27, 2023 1:00 PM	Ended: Jun 27, 2023 1:23 PM

pages-links.spec.js • 14 failed tests

View Output Video

Test		Artifacts
Page Link Handling > Link handling to collectives > Opens link with URL to page in this collective in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to collectives > Opens link with absolute path to page in this collective in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to collectives > Opens link with relative path to page in this collective with fileId in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to collectives > Opens link with relative path to page in this collective with fileId and outdated path in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to collectives > Opens link with relative path to page in this collective without fileId in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to collectives > Opens link with relative path to markdown file in this collective without fileId in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to collectives > Opens link with URL to page in other collective with fileId in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to collectives > Opens link with absolute path to page in other collective without fileId in same/new tab depending on view/edit mode		`Output` `Screenshots`
Page Link Handling > Link handling to Nextcloud > Opens link with URL to another Nextcloud app in new tab		`Output` `Screenshots`
Page Link Handling > Link handling to Nextcloud > Opens link with absolute path to another Nextcloud app in new tab		`Output` `Screenshots`
The first 10 failed tests are shown, see all 14 tests in Cypress Cloud.

pages.spec.js • 1 failed test

View Output Video

Test		Artifacts
Page > Creating a page from template > New page has template content		`Output` `Screenshots`

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

nimishavijay

Very cool! General idea looks great! :) Some points from my end:

It would be useful to show the path to the pages as well, as I can see a use case where 2 pages with the same name have been deleted and then it's difficult to tell which one to restore. I also have concerns about the height of the list when there are more pages. How about a modal like this (adapted from the calendar trash bin)? There is a subline with the page "location" shown
Agreed that we can highlight the deleted pages when the page is deleted! Opening the trash is a bit much indeed
Similarly when a page it restored it can also be highlighted for a couple of seconds
if the page being restored is a subpage of a page that is collapsed in the list then it could automatically be expanded upon restoring

What do you think? :) also ccing @jancborchardt and @szaimen for more design feedback

jancborchardt

Totally agree with @nimishavijay's proposal, modal is more proper here. :)

mejo- · 2023-05-31T13:36:28Z

Thanks a lot for your feedback @nimishavijay and @jancborchardt. I implemented most of your suggestions. See the updated sceenshots/casts.

Only remaining is how to represent the path of the page in trashbin. In the files trashbin, original path is not displayed either. And it can become quite long and cluttered soon if we display it. That's why I went without first, but would be curious about your ideas.

jancborchardt

Nice! Some further feedback:

It is a bit strange that "Delete page" is not the last entry of the action menu. The "last modified" info entry could be moved to first position instead?
Pages with subpages also seems to remove all subpages. Maybe in that case it is ok to have a confirmation modal?
The feedback with highlighting the "Deleted pages" is not super obvious. Generally we should always also use showNotification, in this case it could say "[Pagename] deleted", ideally even with an " Undo" button.
In the modal:
- The table headings don't need to be bold
- "Restore" button could be outside of the menu so it's quick to access, in secondary button style.
- The last horizontal line below the last list item is not necessary

mejo- · 2023-05-31T14:51:55Z

Thanks @jancborchardt! Some quick follow-ups:

Pages with subpages also seems to remove all subpages. Maybe in that case it is ok to have a confirmation modal?

Well, it was forbidden before, but now that it's easy to restore I thought it would be ok to just allow it. But if you prefer, we can add a confirmation modal for sure.

It is a bit strange that "Delete page" is not the last entry of the action menu. The "last modified" info entry could be moved to first position instead?

I'm not sure whether I like that. I will give it a try, but in general the "last modified" info is really more something like an info and not something we should emphasize by putting it as the first entry, no?

jancborchardt · 2023-05-31T16:20:56Z

Thanks @jancborchardt! Some quick follow-ups:

Pages with subpages also seems to remove all subpages. Maybe in that case it is ok to have a confirmation modal?

Well, it was forbidden before, but now that it's easy to restore I thought it would be ok to just allow it. But if you prefer, we can add a confirmation modal for sure.

Yeah, it's just that it's not super clear that this is what happens, at least before you do it. So yes I would say a confirmation is in order here.

It is a bit strange that "Delete page" is not the last entry of the action menu. The "last modified" info entry could be moved to first position instead?

I'm not sure whether I like that. I will give it a try, but in general the "last modified" info is really more something like an info and not something we should emphasize by putting it as the first entry, no?

Right, yeah and actually the "Convert with pandoc" is also a bit of a nerdy option (at least wording-wise)? What do you think about this order:

Select emoji
Move page
Convert with pandoc
Add template for subpages
(edited by info entry)
Delete page

mejo- · 2023-05-31T16:32:15Z

Right, yeah and actually the "Convert with pandoc" is also a bit of a nerdy option

Haha, yeah sorry this is from a custom extension (https://github.com/mejo-/pandoc) and not part of Collectives.

jancborchardt · 2023-05-31T21:51:05Z

Haha, yeah sorry this is from a custom extension (https://github.com/mejo-/pandoc) and not part of Collectives.

Nice! :) May I suggest "Convert or export" (or "Convert & export")?

jancborchardt

Very nice! I would still say the "modified" info line should move one up in the menu so delete is the last entry, but that can also be a follow-up. :)

juliusknorr

Only a minor comment, non blocking, otherwise the code looks good to me. Haven't tested yet though.

juliusknorr · 2023-06-20T19:15:20Z

Tested and found a few issues that might be worth to look into before merging:

Page not removed from trashbin UI list after deleting it permanently
Delete permanently breaks collective, steps to reproduce:
- Create a collective
- Add 1 subpage
- Delete the sub page
- Permanently delete the subpage
- Reload the collective -> See Page not found: {page}
When having a page open while deleting the main view should navigate to the parent page, otherwise you may end up with a broken text editor as the file is no longer present or can continue edit a deleted page
Deleting a page does not immediately remove it from the list
Restoring a page failes if the page has been reloaded (might also happen if the previous would be the case):

mejo- · 2023-06-26T16:12:24Z

Thanks for testing @juliushaertl. I was unable to reproduce most of the issues you face. The problem with broken collective was a severe bug though that I fixed in 3abce81 now. Maybe you can test again?

Page not removed from trashbin UI list after deleting it permanently

For me, pages always get removed from the trashbin UI after deleting permanently

When having a page open while deleting the main view should navigate to the parent page, otherwise you may end up with a broken text editor as the file is no longer present or can continue edit a deleted page

That's what happens in my tests.

Deleting a page does not immediately remove it from the list
Restoring a page failes if the page has been reloaded (might also happen if the previous would be the case):

Mh, unable to reproduce these two as well.

juliusknorr · 2023-06-27T10:57:07Z

Strange, now it works all fine. I'm wondering if I just have been on an outdated branch, though I'm quite sure I pulled before testing.

Anyways, works like a charm now ❤️

Fixes: #47 Signed-off-by: Jonas <[email protected]>

Signed-off-by: Jonas <[email protected]>

We want the subpage order to stay consistent after restore Signed-off-by: Jonas <[email protected]>

* Highlight trash button when trashing a page * Expand path to restored page in page list * Highlight and scroll into view restored page in page list * Don't remove subpage order of parent when trashing a page Signed-off-by: Jonas <[email protected]>

Signed-off-by: Jonas <[email protected]>

* Distinguish 'Delete page' and 'Delete page and subpages' * Fix stickyness of page trash button on mobile * Show notification toast when trashing a page * Remove trailing horizontal line in page trash modal * Don't use bold font for table headings in trash modal Signed-off-by: Jonas <[email protected]>

* Define thead and tbody areas in page trash table * Always show restore button inline in page trash table Signed-off-by: Jonas <[email protected]>

Signed-off-by: Jonas <[email protected]>

If the folder has an index page, we want to use its fileId in the collectives page database. But if it doesn't, we don't want to fail hard. Signed-off-by: Jonas <[email protected]>

Signed-off-by: Jonas <[email protected]>

* Clear timeout before setting a new one * Remove highlight class and add it again for re-highlighting * Clear timeout on component unmount to prevent memory leaks Signed-off-by: Jonas <[email protected]>

mejo- added enhancement New feature or request 2. developing labels May 30, 2023

mejo- requested review from jancborchardt and nimishavijay May 30, 2023 14:46

mejo- force-pushed the enh/pages_trash_frontend branch from c681193 to 9ca6abc Compare May 30, 2023 15:01

nimishavijay requested changes May 30, 2023

View reviewed changes

jancborchardt reviewed May 31, 2023

View reviewed changes

mejo- force-pushed the enh/pages_trash_frontend branch from 0f54482 to cffedb9 Compare May 31, 2023 13:32

mejo- requested review from jancborchardt and nimishavijay May 31, 2023 13:39

jancborchardt requested changes May 31, 2023

View reviewed changes

mejo- force-pushed the enh/pages_trash branch 3 times, most recently from b03b9fb to 34b080a Compare June 5, 2023 14:17

delete-merged-branch bot deleted the branch main June 5, 2023 14:28

mejo- force-pushed the enh/pages_trash_frontend branch from cffedb9 to 3be9342 Compare June 6, 2023 10:45

mejo- changed the base branch from enh/pages_trash to main June 6, 2023 10:49

mejo- requested a review from jancborchardt June 6, 2023 10:50

mejo- marked this pull request as ready for review June 6, 2023 10:56

mejo- requested review from juliusknorr and max-nextcloud June 6, 2023 10:56

mejo- added 3. to review and removed 2. developing labels Jun 6, 2023

jancborchardt approved these changes Jun 6, 2023

View reviewed changes

juliusknorr reviewed Jun 20, 2023

View reviewed changes

mejo- force-pushed the enh/pages_trash_frontend branch from f0e7de2 to 3abce81 Compare June 26, 2023 16:09

mejo- force-pushed the enh/pages_trash_frontend branch from 3abce81 to 090f263 Compare June 26, 2023 16:12

mejo- requested a review from juliusknorr June 26, 2023 16:46

mejo- force-pushed the enh/pages_trash_frontend branch 2 times, most recently from 499a015 to 33390fb Compare June 26, 2023 17:23

juliusknorr approved these changes Jun 27, 2023

View reviewed changes

mejo- force-pushed the enh/pages_trash_frontend branch 4 times, most recently from f3315e2 to d7c6cb8 Compare June 27, 2023 12:55

mejo- added 11 commits June 27, 2023 17:56

Add page trash user interface

c9f7775

Fixes: #47 Signed-off-by: Jonas <[email protected]>

Allow to trash pages with subpages

cdea171

Signed-off-by: Jonas <[email protected]>

Don't remove page from subpage order of parent when trashed

8f6f895

We want the subpage order to stay consistent after restore Signed-off-by: Jonas <[email protected]>

Move page trash into a dedicated modal

21c0e18

Signed-off-by: Jonas <[email protected]>

Table structure and inline restore button

9c1517c

* Define thead and tbody areas in page trash table * Always show restore button inline in page trash table Signed-off-by: Jonas <[email protected]>

Only fetch page trash if user has edit permissions

caae7b5

Signed-off-by: Jonas <[email protected]>

Fix deleting and restoring folders without index page from trash

53825d4

If the folder has an index page, we want to use its fileId in the collectives page database. But if it doesn't, we don't want to fail hard. Signed-off-by: Jonas <[email protected]>

fix(backend): Don't run revertSubFolders() on collective rootfolder

57d1c44

Signed-off-by: Jonas <[email protected]>

Outsource duplicate css animation code into its own scss file

657da5e

Signed-off-by: Jonas <[email protected]>

mejo- force-pushed the enh/pages_trash_frontend branch from d7c6cb8 to 2cfa45e Compare June 27, 2023 15:58

Improve highlight logic for page trash button

43d1d13

* Clear timeout before setting a new one * Remove highlight class and add it again for re-highlighting * Clear timeout on component unmount to prevent memory leaks Signed-off-by: Jonas <[email protected]>

mejo- force-pushed the enh/pages_trash_frontend branch from 2cfa45e to 43d1d13 Compare June 27, 2023 16:07

mejo- merged commit 50ca324 into main Jun 27, 2023

delete-merged-branch bot deleted the enh/pages_trash_frontend branch June 27, 2023 16:19

Add page trash frontend #709

Add page trash frontend #709

Uh oh!

Conversation

mejo- commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Summary

🖼️ Screenshots and screencasts

🚧 TODO

🏁 Checklist

Uh oh!

cypress bot commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

15 failed tests on run #825 ↗︎

Uh oh!

nimishavijay left a comment • edited by mejo- Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jancborchardt left a comment

Choose a reason for hiding this comment

Uh oh!

mejo- commented May 31, 2023

Uh oh!

jancborchardt left a comment • edited by mejo- Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mejo- commented May 31, 2023

Uh oh!

jancborchardt commented May 31, 2023

Uh oh!

mejo- commented May 31, 2023

Uh oh!

jancborchardt commented May 31, 2023

Uh oh!

jancborchardt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Jun 20, 2023 • edited by mejo- Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mejo- commented Jun 26, 2023

Uh oh!

juliusknorr commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mejo- commented May 30, 2023 •

edited

Loading

cypress bot commented May 30, 2023 •

edited

Loading

nimishavijay left a comment •

edited by mejo-

Loading

jancborchardt left a comment •

edited by mejo-

Loading

jancborchardt left a comment •

edited

Loading

juliusknorr commented Jun 20, 2023 •

edited by mejo-

Loading