-
Notifications
You must be signed in to change notification settings - Fork 173
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
Additional link to picture menu #1645
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1645 +/- ##
=========================================
Coverage 34.06% 34.06%
Complexity 99 99
=========================================
Files 14 14
Lines 273 273
=========================================
Hits 93 93
Misses 180 180 Continue to review full report at Codecov.
|
Actually the button being invisible below there is not by design. :) What should happen is that the action menu shows in the top right of the viewer, next to the x button. Then clicking the image anywhere just opens the image, and from there it can be edited. |
Ok, sure. :) Then let's do it like Facebook or the left example of Google do it: Overlaying the menu button on the bottom right, and having an image filetype icon. |
I liked the other implementation from google, because it is more decent (only shown on mouse over). It's at the spot where you are looking for it and at the same time not disturbing in regular operation. Are you sure you prefer the bubble? |
Sorry – you are right, I meant to say it’s good to only show the bubble on hover/focus of the picture. :) (Make sure focus also works for keyboard accessibility.) (Closing was accidental. :) |
Yes, almost perfect – only that the action menu should open from the overlaid image button, not from this invisible action button below the image. :) |
Should we merge the buttons? Maybe a css pro can help? |
I have tested it with Firefox and Chromium. Looking forward to your comments. |
4c2683d
to
16ed121
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.
Looks very good now, great job @call-me-matt! :)
Needs another code review @nextcloud/contacts
Nice!! |
Is thsi ready for reviews or just a draft? |
It needs a code review from someone who knows scss, I'd say |
Please rebase @call-me-matt I'll do another review on this once we have the menu back in the Modal too :) |
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 :)
Also, don't forget to squash and use fixups. We're already at 12 commits ;) |
af9c2c4
to
4d0df73
Compare
Design question (@jancborchardt) |
Yes :p |
I realized the "chose from files" icon is an image in the avatar-menu and a folder on the modal-menu. Which one to take? |
Let's leave it like that for now? @jancborchardt |
Signed-off-by: call-me-matt <nextcloud@matthiasheinisch.de>
4d0df73
to
e6a1199
Compare
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/contacts/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
It took me a while to find the menu where I can delete contact pictures, so here is a proposal to add an overlay and make it more visible to users. Also: moved and merged the menu from the modal, which currently has different menu entries than the button.
Related issues: #1513
Implementation is very rudimentary, just to give you an idea. Next steps would be to optimize the style (edit or photo icon instead of text, chose color/transparency) and then implement it well in the scss.