-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add download button #814
Add download button #814
Conversation
c144122
to
c421682
Compare
I do not immediately see why Cypress is failing. Does anyone have some pointers for me? |
Thanks for continuing this! |
The material design guidelines specify that separator should be used to "to create groupings rather than separate items" . I personally don't feel like "open sidebar" and "download" would form a group as they are very different actions. What do you think @szaimen ? |
That's a good guideline 👍 I just wanted to bring this up as an enhancement point but the better way (instead of a separator) would probably be to ask if you want to delete a file and not instantly deleting it. So I guess it is up to the @nextcloud/designers team to decide if a separator makes sense here... |
Oh, It should not instantly delete the file in the viewer without confirmation. That would be very bad, is there an issue open for that already? edit: I read some more about this and there is a an open issue for this: #746 |
Hey, nice addition! What about having the download button always visible instead of within the actions menu? I think it would be very handy. |
@ma12-co not available in the current Modal component, but that would be nice indeed :) |
So then how to move this PR forward? |
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.
See comments :)
Sorry it took me time, very nice work! 🚀
/backport to stable21 |
/backport to stable20 |
/backport to stable19 |
cb9728a
to
28e231b
Compare
Got it working locally. I needed to add a
(also works in the check run: https://github.com/nextcloud/viewer/pull/814/checks?check_run_id=2207888337#step:8:456 ) |
d64dc07
to
cb259a5
Compare
d2ce935
to
b44b163
Compare
@skjnldsv, some cypress tests are failing, but I don't know why. Could you take a look at it? |
b44b163
to
6d0deb3
Compare
Relevant bug |
Pfiouu, lots of fixes!
Again, big thank you for all the work @beardhatcode and @fflorent. Couldn't have found the time if you had not done 99% of the work! 🎉 |
bb9312f
to
ed84fd3
Compare
This comment has been minimized.
This comment has been minimized.
ed84fd3
to
64c37f6
Compare
64c37f6
to
ed84fd3
Compare
Signed-off-by: Florent Fayolle <florent.github-contributions@zeteo.me> Signed-off-by: Robbert Gurdeep Singh <git@beardhatcode.be>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
ed84fd3
to
d6dabe9
Compare
Everything passes locally 🤔 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
All good on master https://github.com/nextcloud/viewer/actions/runs/732228778 |
I will stop at 21 unfortunately, 20 might be too different, let's see on #849 |
Continuation of #702.
I addressed @skjnldsv comments and moved the download button to above the delete button (it felt better to me like that).
Works on my server and in the docker image.
Mentioning relevant people:
@fflorent @skjnldsv