Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Navigate via folder tree #46596

Merged
merged 11 commits into from
Aug 1, 2024
Merged

feat: Navigate via folder tree #46596

merged 11 commits into from
Aug 1, 2024

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented Jul 18, 2024

Summary

image

  • Allow toggling folder tree

Split commits for easier reviewing :)

Checklist

@Pytal Pytal added enhancement 2. developing Work in progress feature: files feature: file sidebar Related to the file sidebar component labels Jul 18, 2024
@Pytal Pytal added this to the Nextcloud 30 milestone Jul 18, 2024
@Pytal Pytal self-assigned this Jul 18, 2024
@Pytal Pytal removed the feature: file sidebar Related to the file sidebar component label Jul 25, 2024
@Pytal Pytal changed the title feat: Add folder tree to sidebar feat: Navigate via folder tree Jul 25, 2024
@Pytal Pytal marked this pull request as ready for review July 25, 2024 02:08
@Pytal Pytal requested a review from skjnldsv as a code owner July 25, 2024 02:08
@Pytal Pytal added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 25, 2024
@Pytal Pytal enabled auto-merge July 25, 2024 02:50
@sorbaugh sorbaugh mentioned this pull request Jul 25, 2024
@sorbaugh sorbaugh requested a review from Fenn-CS July 25, 2024 07:18
apps/files/src/services/FolderTree.ts Outdated Show resolved Hide resolved
apps/dav/lib/Connector/Sabre/FilesPlugin.php Outdated Show resolved Hide resolved
apps/files/lib/Controller/ViewController.php Outdated Show resolved Hide resolved
apps/files/src/actions/openFolderAction.ts Outdated Show resolved Hide resolved
apps/files/src/actions/openFolderAction.ts Outdated Show resolved Hide resolved
apps/files/src/components/FileEntry/FileEntryName.vue Outdated Show resolved Hide resolved
lib/private/Files/Node/Folder.php Outdated Show resolved Hide resolved
skjnldsv

This comment was marked as resolved.

susnux

This comment was marked as resolved.

@Pytal Pytal force-pushed the feat/folder-tree branch 2 times, most recently from 68ea1ba to 662d3ba Compare July 25, 2024 23:52
@skjnldsv skjnldsv removed the 3. to review Waiting for reviews label Jul 26, 2024
@blizzz blizzz mentioned this pull request Aug 1, 2024
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments to be addresses later 👍

@sorbaugh sorbaugh disabled auto-merge August 1, 2024 16:12
@sorbaugh sorbaugh dismissed icewind1991’s stale review August 1, 2024 16:13

Will address in a separate PR!

@Pytal Pytal enabled auto-merge August 1, 2024 16:23
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 1, 2024
@Pytal Pytal added the pending documentation This pull request needs an associated documentation update label Aug 1, 2024
@blizzz blizzz disabled auto-merge August 1, 2024 18:47
@blizzz blizzz merged commit ef7d830 into master Aug 1, 2024
165 of 169 checks passed
@blizzz blizzz deleted the feat/folder-tree branch August 1, 2024 18:48
@skjnldsv
Copy link
Member

skjnldsv commented Aug 1, 2024

Congrats 🎉 🎉 🎉

@nickvergessen
Copy link
Member

The #[ApiRoute(verb: 'GET', url: '/api/v1/folder-tree')] takes down our server and makes browsers freeze.
Had to shortcut it with:

		return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR, [], JSON_FORCE_OBJECT);

@frake65
Copy link

frake65 commented Aug 7, 2024

The #[ApiRoute(verb: 'GET', url: '/api/v1/folder-tree')] takes down our server and makes browsers freeze. Had to shortcut it with:

		return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR, [], JSON_FORCE_OBJECT);

Finally this shortcut works for me.

What exactly should the code called in function "getFolderTree()" be doing? - Implement the new feature side bar?
Nice idea, but we would need a more performant implemetation for that feature - and probably another user toggle to have it turned on or off.
If it stays as expensive as the current implementation (at least for larger file trees), another solution could be to add the file tree asynchrosely and have the directory view in place instantly.

@Pytal
Copy link
Member Author

Pytal commented Aug 7, 2024

We are working on optimizing this feature @frake65, though the page might be too unresponsive with very large trees there is an "Enable folder tree" checkbox located in the Files settings dialog which can be toggled per-user and will prevent the endpoint from being called completely

@frake65
Copy link

frake65 commented Aug 8, 2024

I did remove the shortcut and now I'm using the toggle switch. - Thank you for pointing me there!
Sorry to say, but this implementation seems to be still buggy to me - not only slow:

  • From a users view point at this time I get a file tree, where folder names do not match the folders that they link to at all, but have random names (name of a random folder within the sub tree they are pointing to).
  • What should be the order of the folder names by the way? - Alphabetically would be my first guess.
  • minor but still wrong: the german tranlation is missing in file "l10n/de.js" and "l10n/de.json" - so the toggle is named in english language for me.

In my opinion probably this feature should be postponed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: files pending documentation This pull request needs an associated documentation update 🍂 2024-Autumn
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a folder tree sidebar
10 participants