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

Prevent progress bar overflow when uploading in Folders #1785

Merged
merged 1 commit into from
May 16, 2023

Conversation

artonge
Copy link
Collaborator

@artonge artonge commented May 10, 2023

Fix #1682

Before After
Screenshot from 2023-05-16 14-59-01 Screenshot from 2023-05-16 15-05-28
Screenshot from 2023-05-16 14-59-30 Screenshot from 2023-05-16 15-05-03

@nextcloud/designers any idea on how to fix that nicely?

@artonge artonge enabled auto-merge May 10, 2023 14:45
@artonge artonge self-assigned this May 10, 2023
@artonge artonge force-pushed the artonge/fix/progress_bar branch from 3c85df5 to 4f03c60 Compare May 10, 2023 14:45
@artonge artonge added bug Something isn't working 3. to review Waiting for reviews javascript Javascript related ticket labels May 10, 2023
@artonge artonge added this to the Nextcloud 27 milestone May 10, 2023
@skjnldsv
Copy link
Member

Would it make sense to upstream to @nc/uploader ?

@artonge
Copy link
Collaborator Author

artonge commented May 11, 2023

Would it make sense to upstream to @nc/uploader ?

Depending on the final solution, yes, but not satisfied by the current one. Let's see what @nextcloud/designers have to say :)

@nimishavijay
Copy link
Member

Great catch! What do you think about something like this?

image

Over here, only on mobile there is some extra space below the add button where the progress bar would appear. This gives ample space for longer text and makes sure that no other elements jump around during upload. What do you think? cc @szaimen

@szaimen
Copy link
Contributor

szaimen commented May 11, 2023

Yes, I also think that moving it to the next line on mobile is the way to go 👍

@szaimen
Copy link
Contributor

szaimen commented May 11, 2023

But I would not add a big empry space in case the upload bar is not visible

@artonge
Copy link
Collaborator Author

artonge commented May 11, 2023

@skjnldsv I do not see any other way than emitting a progress event from @nc/uploader and recreate a progress bar here. Maybe we can create and export the progress bar component in @nc/uploader to not duplicate code. What do you think?

@skjnldsv
Copy link
Member

@skjnldsv I do not see any other way than emitting a progress event from @nc/uploader and recreate a progress bar here. Maybe we can create and export the progress bar component in @nc/uploader to not duplicate code. What do you think?

That would kind of defeat the purpose of having a simple and universal component look.

@nimishavijay the uploader design is aiming to be a standard in other locations of Nextcloud. If we start putting the progress in another location (like bellow the title), it won't work well with what we're trying to achieve I'm afraid 🤔

@nimishavijay
Copy link
Member

Hmm, are there examples of other places where the component is/will be used? We can try and come to a design that works for most use cases? :)

@skjnldsv
Copy link
Member

skjnldsv commented May 12, 2023

@nimishavijay Files app for example or like Talk that is not using this component yet :)
image
image

@artonge please also note that we do expose current list and progress.

@nimishavijay
Copy link
Member

Hmm, even in Files and Talk, for the mobile view the best place does seem to be below the add/upload button/whatever element the uploader is associated with. Any other placement will give rise to the same problem as we saw. Is it totally not possible to standardise this design?

Some alternatives would be to use a loading spinner instead of a progress bar on mobile, or maybe showing the uploading message in a toast notification?

@skjnldsv
Copy link
Member

skjnldsv commented May 16, 2023

@nimishavijay what would the standard look like?
This is an upload component, not a Title-header-upload component.
So if we want to stay generic, I don't see what else we can do.

Also, and that's my personal opinion, I think your mockup doesn't look polished enough from a UI perspective. The spacing is not consistent (for example; I'm guessing you just did a quick mockup in photoshop), but if we want to really make sure we can move forward, having a real final mockup to refer to for adjustments would be really helpful :)
image

@artonge artonge force-pushed the artonge/fix/progress_bar branch from 4f03c60 to fa424d9 Compare May 16, 2023 12:57
@artonge
Copy link
Collaborator Author

artonge commented May 16, 2023

Hacky solution for now with css positioning. But it works.
Let's wait for Talk and Files to use this component to have more concrete use cases and see how we can unify those.
Screenshot updated.

@artonge artonge disabled auto-merge May 16, 2023 13:09
@artonge
Copy link
Collaborator Author

artonge commented May 16, 2023

/backport to stable26

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label May 16, 2023
@artonge
Copy link
Collaborator Author

artonge commented May 16, 2023

/backport to stable25

@artonge artonge force-pushed the artonge/fix/progress_bar branch from fa424d9 to 6bb7169 Compare May 16, 2023 13:11
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.

LGTM for the meantime (but didnt test)

@artonge artonge merged commit af70a0a into master May 16, 2023
@artonge artonge deleted the artonge/fix/progress_bar branch May 16, 2023 20:38
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 bug Something isn't working javascript Javascript related ticket
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Upload progression bar spread view in photo app
4 participants