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.

Sign up for GitHub

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

Jump to bottom

Make share results distinguishable if there are more than one with the same display name #23017

Merged
merged 4 commits into from
Dec 14, 2020
Merged

Make share results distinguishable if there are more than one with the same display name #23017

merged 4 commits into from
Dec 14, 2020

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Sep 23, 2020 •
edited
Loading

  • Give a context for users that have the same display name in the sharing input and share list
  • The context is the email address if set, otherwise it will fallback to the uid

Keeping current behavior if display names are different

image

Same display name (first two users have an email set)

image

Share listing

image

@juliushaertl juliushaertl added 3. to review Waiting for reviews bug design Design, UI, UX, etc. feature: sharing labels Sep 23, 2020
@juliushaertl juliushaertl added this to the Nextcloud 21 milestone Sep 23, 2020
@juliushaertl juliushaertl added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 23, 2020
@juliushaertl
Copy link
Member Author

@jancborchardt Could you maybe give me some input here? Having the additional bottom line is not ideal as we usually have user status there now, though it is not yet implemented in the sharing autocomplete dialog.

@jancborchardt
Copy link
Member

@jancborchardt Could you maybe give me some input here? Having the additional bottom line is not ideal as we usually have user status there now, though it is not yet implemented in the sharing autocomplete dialog.

@juliushaertl Ah good point – how about putting it directly after the display name? Set as var(--color-text-maxcontrast) so it’s not so harsh. Possibly also parentheses around.

@juliushaertl
Copy link
Member Author

Yep, sounds good, any idea about if we should show the uid or email? quick summary:

  • uid is unique but might be random characters on ldap setup
  • email might not be set for the users with the same displayname or might not be unique

@jancborchardt
Copy link
Member

I would tend towards email since we don’t really use the user id often, so it’s easier to identify people by mail. If it’s not unique, then they are also the same person?

@juliushaertl
Copy link
Member Author

If it’s not unique, then they are also the same person?

Yep indeed 😁 👍

@juliushaertl juliushaertl added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 7, 2020
@juliushaertl
Copy link
Member Author

Addressed the discussion and updated the first post with details about the implementation, ready for review.

MorrisJobke
MorrisJobke approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 7, 2020
MorrisJobke
MorrisJobke previously requested changes Oct 7, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

CI fails.

@juliushaertl juliushaertl force-pushed the enh/distinguish-users branch 2 times, most recently from 3afa35c to d78d2d4 Compare October 13, 2020 14:54
@MorrisJobke
Copy link
Member

/compile /

@MorrisJobke
Copy link
Member

CI still fails

@tobiasKaminsky tobiasKaminsky mentioned this pull request Nov 27, 2020
Open
4 tasks
@MorrisJobke
Copy link
Member

conflicts :(

jancborchardt
jancborchardt approved these changes Dec 10, 2020
View reviewed changes
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.

Great solution design-wise! :)

@juliushaertl
Copy link
Member Author

Rebased and rebuilt.

@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 10, 2020
@juliushaertl
Copy link
Member Author

/backport to stable20

@juliushaertl
Copy link
Member Author

/backport to stable19

@juliushaertl
Copy link
Member Author

/backport to stable18

@rullzer rullzer mentioned this pull request Dec 14, 2020
Merged
59 tasks
@juliushaertl
Make share results distinguishable if there are more than one with th…
a86746e 
…e exact same display name

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Show unique displayname context in the sharing input
131fcac 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Show unique displayname context in the user share list entries
102402b 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Bump bundles
8b13fd4 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

Resolved again

@juliushaertl juliushaertl merged commit b56d7f9 into master Dec 14, 2020
@juliushaertl juliushaertl deleted the enh/distinguish-users branch December 14, 2020 15:54
@backportbot-nextcloud
Copy link

The backport to stable20 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable19 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable18 failed. Please do this backport manually.

This was referenced Dec 29, 2020
Merged
Merged
Merged
skjnldsv
skjnldsv reviewed Mar 11, 2021
View reviewed changes
apps/files_sharing/src/models/Share.js
@@ -150,6 +150,10 @@ export default class Share {
|| this.#share.share_with
}

get shareWithDisplayNameUnique() {
Copy link
Member

Choose a reason for hiding this comment

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

Tss tss tss @juliushaertl , no documentation? 😁 🙈

@nickvergessen nickvergessen mentioned this pull request May 3, 2022
Merged
@nickvergessen nickvergessen mentioned this pull request May 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv left review comments

@jancborchardt jancborchardt jancborchardt approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug design Design, UI, UX, etc. feature: sharing
Projects
None yet
Milestone
Nextcloud 21
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@juliushaertl @jancborchardt @MorrisJobke @ChristophWurst @tobiasKaminsky @skjnldsv @diroots