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

Show the child folders in the breadcrumb menu when on a parent entry. #30925

Conversation

paierlep
Copy link

@paierlep paierlep commented Jan 30, 2022

Previously, clicking on an menu item in the breadcrumb menu removed the
child entries of the path, i.e.:
Clicking on the "to" entry in "/path/to/some/folder" changed the
breadcrumb menu to show only the "/path/to" entries.
With this change the breadcrumb menu changes this behaviour as the full
path is still visible (and usable) but with the "to" entry beeing
highlighted.

Closes #1079
cc @nextcloud/designers @nextcloud/javascript @jancborchardt @szaimen

Signed-off-by: Christian Paier hallo+git@cpaier.com

@szaimen szaimen added 3. to review Waiting for reviews enhancement labels Jan 31, 2022
@szaimen szaimen added this to the Nextcloud 24 milestone Jan 31, 2022
@szaimen szaimen requested review from jancborchardt, nimishavijay, a team, PVince81, artonge and Pytal and removed request for a team January 31, 2022 10:21
@szaimen szaimen added the design Design, UI, UX, etc. label Jan 31, 2022
@nimishavijay
Copy link
Member

Sounds great! Would it be possible to post screenshots of the before/after to make it easier to see the design changes? :)

@paierlep
Copy link
Author

Yes sure:
The breadcrumb renders as usually when viewing a folder:
in_child_folder

Clicking on one of the parent folders gives a slightly different behavior:
clicking_on_parent

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

🚀

@artonge artonge changed the title Show the parent folders in the breadcrumb menu when on a child entry. Show the child folders in the breadcrumb menu when on a parent entry. Feb 1, 2022
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Would it be possible to adjust the gaps so that the gaps in before and after look the same?
image

@nimishavijay
Copy link
Member

There is a new breadcrumbs component, this would just be integrated with that, correct?

@artonge
Copy link
Contributor

artonge commented Feb 1, 2022

There is a new breadcrumbs component, this would just be integrated with that, correct?

No, the new breadcrumbs component is in vue, and this touches legacy code :(.

apps/files/js/breadcrumb.js Outdated Show resolved Hide resolved
apps/files/js/breadcrumb.js Outdated Show resolved Hide resolved
apps/files/js/breadcrumb.js Outdated Show resolved Hide resolved
apps/files/js/breadcrumb.js Outdated Show resolved Hide resolved
apps/files/js/breadcrumb.js Outdated Show resolved Hide resolved
@paierlep
Copy link
Author

paierlep commented Feb 1, 2022

Would it be possible to adjust the gaps so that the gaps in before and after look the same? image

No problem - I've changed the scss file accordingly:

Screenshot from 2022-02-01 21-41-34
Screenshot from 2022-02-01 21-41-57

@paierlep paierlep force-pushed the feature/breadcrumb_menu_parent_folder_visible branch from 17153aa to 66a073a Compare February 1, 2022 20:52
@artonge
Copy link
Contributor

artonge commented Feb 2, 2022

Need a rebase on master to make the continuous-integration/drone/pr work

git rebase origin/master

@artonge artonge requested a review from skjnldsv February 2, 2022 09:30
@paierlep paierlep force-pushed the feature/breadcrumb_menu_parent_folder_visible branch 2 times, most recently from a83250b to 0942aed Compare February 2, 2022 20:17
Previously, clicking on an menu item in the breadcrumb menu removed the
parent entries of the path, i.e.:
Clicking on the "to" entry in "/path/to/some/folder" changed the
breadcrumb menu to show only the "/path/to" entries.
With this change the breadcrumb menu changes this behaviour as the full
path is still visible (and usable) but with the "to" entry beeing
highlighted.

Signed-off-by: Christian Paier <hallo+git@cpaier.com>
@szaimen
Copy link
Contributor

szaimen commented Feb 3, 2022

No problem - I've changed the scss file accordingly:

Screenshot from 2022-02-01 21-41-34 Screenshot from 2022-02-01 21-41-57

Thanks! looks much better to me now :)

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

tested and works 👍
(but didn't review the code)

@paierlep
Copy link
Author

paierlep commented Feb 3, 2022

Need a rebase on master to make the continuous-integration/drone/pr work

Rebasing did not help with sqlite-php7.4-samba-non-native (No code coverage driver is available)

@artonge artonge merged commit 49959bc into nextcloud:master Feb 8, 2022
@welcome
Copy link

welcome bot commented Feb 8, 2022

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@artonge
Copy link
Contributor

artonge commented Feb 8, 2022

Thanks for the PR @paierlep !

@raimund-schluessler
Copy link
Member

There is a new breadcrumbs component, this would just be integrated with that, correct?

No, the nextcloud/vue breadcrumb component does not support this. If this is really desired, we would need to modify it.

But I have to admit, I find the behavior confusing. It takes more attention to figure out, which folder is currently open. And while this is a personal feeling that might change when getting used to it, there is also an objective usability problem with this solution.

See this screenshot. Which folder is currently open?
Screenshot 2022-02-08 at 15-10-42 Even more folders - Files - Nextcloud

The currently open folder is hidden, because the breadcrumb bar is to long. So it is not possible to see which folder is open.

And even clicking the three-dots menu does not give a hint, because the currently open folder ("Even more folders" in this case) is not bold.
Screenshot 2022-02-08 at 15-11-15 Even more folders - Files - Nextcloud

The second point can be fixed by making the folder that is open bold in the three-dots menu. For the first issue, I don't see a solution using this approach, to be honest.
@jancborchardt How does Nautilus handle this?

@paierlep
Copy link
Author

paierlep commented Feb 8, 2022

Thank you for reviewing @artonge, @szaimen, @nimishavijay!

@raimund-schluessler nautilus seems to omit the first folders of the path, for a nested set of folders from a to z the outcome on the folder "u" only shows the folders "f" to "u":
Screenshot from 2022-02-08 20-58-27

Therefore getting to the root folder is only possible by clicking on the "f" folder first.
To add my 2 cents:
I prefer the dropdown menu from the vue screenshot above. Additionally I think it could look something like this:

Home > ... > Active Folder > ... > Deepest Child Folder

With the ... being one of those placeholders with dropdown menu from the screenshot. Best solution would be of course if there is a lot more space to expand this to, i.e.:

Home > ... > Parent Folder > Active Folder > Child Folder > ... > Deepest Child Folder

@raimund-schluessler
Copy link
Member

To add my 2 cents: I prefer the dropdown menu from the vue screenshot above. Additionally I think it could look something like this:

Home > ... > Active Folder > ... > Deepest Child Folder

This only works if there is enough space. But if there is enough space, this issue won't appear. So what you propose makes it a bit better, yes. But it is no real solution, in my opinion.

Especially on mobile, there is often only enough space to show one, maybe two levels. This is iPhone 8 for example:
Screenshot 2022-02-08 at 22-31-18 Long folder name - Files - Nextcloud

If you navigate up on such a device, you will always have to click on the three dots menu to see your active folder.

@jancborchardt
Copy link
Member

So @nimishavijay and I checked back on how it’s done elsewhere. Since I initially opened the issue #1079 because Nautilus did it, checking with it now, they don’t do it anymore. Neither does e.g. Google Drive or Dropbox.

Hence considering all the issues and possible confusion this brings, it’s probably best to not do this after all – @paierlep sorry about that, we should have kept the issue up to date. :\

@raimund-schluessler what do you think?

@paierlep
Copy link
Author

(deleting my previous comment, as I've been stupid and did my comparison wrong ;) )

@jancborchardt Don't worry - I should have mentioned it earlier that I'm working on it ;)

@szaimen
Copy link
Contributor

szaimen commented Mar 11, 2022

So should thid be reverted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When navigating up to parent folder again, breadcrumb should stay for easier navigation back down
7 participants