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

Finishing PR #1154 #1429

Merged
merged 9 commits into from
Oct 29, 2019
Merged

Finishing PR #1154 #1429

merged 9 commits into from
Oct 29, 2019

Conversation

DominiqueFuchs
Copy link
Contributor

See #1154 . Thanks to @IzabelaBakollari for starting this. I recreated her changes with necessary modifications based on the current codebase.

Further additions:

  • Requirements from the original PR added by @jancborchardt (line height)
  • Alignment of inserted widgets in general, see pictures below
  • Fixed wrongly sized userline widget and resulting inaccessibility of buttons

Old (stable 2.5.3):
sharedialog_wo_notes

New:
sharedialog_w_notes

Test note:
TestNote

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
Copy link
Member

@camilasan camilasan left a comment

Choose a reason for hiding this comment

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

It looks great 😍

@jancborchardt
Copy link
Member

Will review in a bit, meanwhile @DominiqueFuchs I sent you an email about a possible Nextcloud desktop hackweek to your uni email (hope that’s ok! :D)

@jancborchardt
Copy link
Member

Looks reeeally nice! :) Only some details for now:

  • Why does the "Shared with you by John Doe" only pop up when you share it yourself to someone else?
  • Would it be possible to make the background white instead of grey? Would make it look a bit more modern. :)

@DominiqueFuchs
Copy link
Contributor Author

@jancborchardt

  1. The "shared with you" line is immediately displayed if the share dialog is opened by a user that received the shared file (1st screenshot below, here I view the file as Jane after sharing it by John. The gif files from the initial posts start with a file that is not yet shared with anyone, so IMHO it's UX-wise correct to add the 'is shared' inidicator only after adding a user or group to share with. However, maybe this line can be optimized for the view of the sharing user. For example, I implemented an alternative if-case where the uid of the sharer equals the current uid (2nd screenshot below)

  2. If possible, I think the coloring would fit better in a seperate (TODO) issue/PR, bc we do no actual theming in the client right now. Setting the color to explicit white here would probably break KDE theming, which in turn would result in ugly and temporary ifdefs/hacks in the code. So my proposal is to do this seperately in the future by implementing a 'real' centralized theming mechanism that respects platform specific dark/white modes (as are now present in Linux, macOS and Windows) or optional user setting. But as always: only IMHO. I did hoewever remove the white autofill for the userline (was this intended to provide visual separation? If yes, I'd rather set a thin frame or something) as you can see in the screens below.

sharedFileFromJohn_JanesView

sharedFileFromJane_JanesView

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@camilasan
Copy link
Member

camilasan commented Sep 19, 2019

  1. If possible, I think the coloring would fit better in a seperate (TODO) issue/PR, bc we do no actual theming in the client right now. Setting the color to explicit white here would probably break KDE theming, which in turn would result in ugly and temporary ifdefs/hacks in the code. So my proposal is to do this seperately in the future by implementing a 'real' centralized theming mechanism that respects platform specific dark/white modes (as are now present in Linux, macOS and Windows) or optional user setting. But as always: only IMHO. I did hoewever remove the white autofill for the userline (was this intended to provide visual separation? If yes, I'd rather set a thin frame or something) as you can see in the screens below.

Totally 👍

@jancborchardt
Copy link
Member

1. The "shared with you" line is immediately displayed if the share dialog is opened by a user that received the shared file (1st screenshot below, here I view the file as _Jane_ after sharing it by _John_. The gif files from the initial posts start with a file that is not yet shared with anyone, so IMHO it's UX-wise correct to add the 'is shared' inidicator only after adding a user or group to share with. However, maybe this line can be optimized for the view of the sharing user. For example, I implemented an alternative if-case where the uid of the sharer equals the current uid (2nd screenshot below)

Ok, so here’s the logic as per the sidebar in the web interface:

  • The "You shared this file" which you show in the latest screenshot above we never need. :) It shouldn’t show anything there if you are the original owner of the file.
  • If you got the file from anyone else first, this line says things like:
    image
    image

So the case that a sentence only appears after you shared a file with anyone will never happen, cause it is only an indicator with how this file was shared to you.

Does that make sense?


Coloring is fine for a separate pull request, yep. :) Good on removing the white for the user fill!

@DominiqueFuchs
Copy link
Contributor Author

Thanks @jancborchardt , I got it now - comparing this to the nc server flow would have been a good idea. 😬

I shortened down the if statement again, but including an additional requirement for the current user not being the one who shares the file to display the "shared by you with ..." message (which showed an "shared with you by 'your own name' in the original code). Now it immediately display a line if you got the file from someone else but nothing if you are the original owner.

@DominiqueFuchs DominiqueFuchs mentioned this pull request Sep 24, 2019
6 tasks
@misch7 misch7 added this to the 2.7 🌟 UI improvements milestone Oct 5, 2019
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

🚀

@jancborchardt
Copy link
Member

@DominiqueFuchs what about the Drone failure? :)

@DominiqueFuchs
Copy link
Contributor Author

Sorry, I messed up while resolving the conflicts. Will do in the following days and then merge 👍

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>
@DominiqueFuchs DominiqueFuchs merged commit d18e2ba into master Oct 29, 2019
@DominiqueFuchs DominiqueFuchs deleted the recipientnote branch October 29, 2019 13:49
@jancborchardt
Copy link
Member

Good stuff @DominiqueFuchs! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants