-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(breadcrumbs): improved rendering of breadcrumb progress bar #44450
fix(breadcrumbs): improved rendering of breadcrumb progress bar #44450
Conversation
7774bfc
to
a4cae4e
Compare
Should we not always show the breadcrumbs? Otherwise mobile users can not navigate, no? Maybe instead of breadcrumbs show a select component with the paths as option on small mobile? |
a4cae4e
to
381363f
Compare
@susnux I changed the title and description of the PR to match the changes you suggested now! On mobile / small screens, we just wrap the progress bar on a newline now :) Reasoning for the 512px is because we use this size here : server/apps/files/src/views/FilesList.vue Line 29 in 7d51b6f
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works!
But this makes a bug of the breadcrumbs more visible: They overflow their container sometimes. This also happens on master but is very visible now.
(But needs to be fixed on the components part)
Yup, I agree @susnux . Just testing it out, and the overflow can get quite clunky here and there. The solution you proposed on the design team is good to me! firefox_SWtd1aJUhD.mp4Here is a video in case you need to showcase what you mean to designers. |
What is missing is making the uploader "new" button icon only on <370px width. But this needs to be fixed on the library I think. |
Considering the changes and the discussion here, please loop the design team in before any approval or merge :) |
Already done in design chat |
62f91fd
to
3a92ff3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this would be a mock of our smallest supported width of 300px, here I made the new-button icon only and reduced the min-width of the last breadcrumb to 72px so it does not overlap with the button
Looks good @susnux – I would even say that "All files" can get moved into the 3-dot menu before the "New" needs to get cut from the button label, as it saves a bunch more space.
@jancborchardt That can definitely be done! Just needs to be changed here within this logic, AFAIK :) : However, just as a counterpoint, the last breadcrumb already just shows the user the directory they are in. Clicking on it just would take them to the same place - so isn't it more useful to have all files stay? Plus, we would be able to keep the cool icon :) I do not have a huge opinion on this, so whatever you decide is best :) |
So AFAIK, the changes that are needed for the breadcrumb are all library-dep changes.... so for the time being, anyone want to give this PR a green check 👀💚 |
It not only is a link though, it also is the only place where we show the name of the folder you are in. And that is more important to know than to quickly go back to "All files". :) (Good for asking though!) |
So I would say we start with this one and the either move the "all files" into the overflow menu (nextcloud-vue changes) or make the "new +" button icon only (nextcloud-upload change) later? |
I agree! This PR gets the mobile view to have correct reactivity, then we can improve it even further by what you mentioned. I will tackle the nextcloud-vue library, do you want to tackle the upload-library? |
3a92ff3
to
fb8fdd8
Compare
fb8fdd8
to
96e798f
Compare
96e798f
to
889105a
Compare
Related cypress
|
Weird, we did not change anything that should be breaking the cypress 🙈 Will investigate and re-run to see if what works! EDIT: Trying a rebase?? Cypress seems weird rn... |
889105a
to
955bd3e
Compare
Cypress seems to be related |
955bd3e
to
7ba8e8e
Compare
Signed-off-by: Eduardo Morales <emoral435@gmail.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de> Signed-off-by: Eduardo Morales <emoral435@gmail.com>
Signed-off-by: Eduardo Morales <emoral435@gmail.com>
Signed-off-by: Eduardo Morales <emoral435@gmail.com>
7ba8e8e
to
c252ee5
Compare
The backport to # Switch to the target branch and update it
git checkout stable29
git pull origin stable29
# Create the new backport branch
git checkout -b backport/44450/stable29
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick de47a9ef f6b1fd41 0213fb6b c252ee55
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/44450/stable29 Error: Failed to push branch backport/44450/stable29: fatal: could not read Username for 'https://github.com': No such device or address Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
/backport to stable29 |
Summary
See #44162 (comment) for context on this PR.
Essentially, users currently cannot browse and navigate to other files while an upload is in progress, so this change fixes the logic dedicated to rendering the breadcrumbs.
We now only hide breadcrumbs when the width of the file list is 400px AND when there is an upload in progress.
firefox_bkEM8LLGhh.mp4
Checklist