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

Sharing user consolidation #11899

Merged
merged 24 commits into from
Oct 30, 2018
Merged

Conversation

juliusknorr
Copy link
Member

  • Share list design improvements
    • Highlight matched strings
    • Merge contacts with the same uuid and group them in the ui
    • Increase the max-width of the suggestion box
    • Add icons for easier recognition of the share type
  • Share search improvements
    • Remove federated sharing address books which are the same as local ones
    • Filter out local users from address book remote searches
    • Filter out generic remote result for local users
    • Add uuid to results to allow proper grouping and avatar placeholder generation
    • Add type of property to the ContactsManager search result (e.g. HOME/WORK for EMAIL)

Before (with increased height for better visibility):
image

After:

image

cc @nextcloud/designers for early feedback 😉

@jancborchardt
Copy link
Member

So in the screenshot, the "Contacts example 2" is a person you can share federated or via 2 emails with, right?

For real consolidation, we should completely collapse it into one entry/row. We only need to show the preferred share method, which in this case is federated sharing.

And the priority is: same Nextcloud > federated cloud ID > email.

@juliusknorr
Copy link
Member Author

So in the screenshot, the "Contacts example 2" is a person you can share federated or via 2 emails with, right?

Yes, "Contacts example 2" is an entry in the address book that has 2 email addresses and a federated sharing id.

For real consolidation, we should completely collapse it into one entry/row. We only need to show the preferred share method, which in this case is federated sharing.

What about a contact, that has a personal cloud id and a personal and work email address. You would not be able to share it to the work related email then?

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Looks amazing! :)
Just a very small nitpick ;)

/* limit dropdown height to 4 1/2 entries */
max-height: calc(36px * 4.5);;
/* limit dropdown height to 7 entries */
max-height: calc(36px * 7);
Copy link
Member

Choose a reason for hiding this comment

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

Please use a half one to ensure the user know the list is not over :)

@skjnldsv
Copy link
Member

For real consolidation, we should completely collapse it into one entry/row. We only need to show the preferred share method, which in this case is federated sharing.

Keep in mind that dropdown should be the simplest UX process. One click searching, one click selecting. If after selecting a contact you need to after click another stuff to specify email or federated, this is a bad behaviour I think 🤔
Not sure if that was what you were suggestion, but adding my comment just in case 😉. If I misunderstood, please ignore :)

@juliusknorr
Copy link
Member Author

Keep in mind that dropdown should be the simplest UX process. One click searching, one click selecting. If after selecting a contact you need to after click another stuff to specify email or federated, this is a bad behaviour I think thinking
Not sure if that was what you were suggestion, but adding my comment just in case wink. If I misunderstood, please ignore :)

From my understanding it was about not showing any other sharing methods of a user at all. But I'm a bit worried about the missing options to share to some other contact point as already said above.

@jancborchardt Also how would we decide which contact information is the relevant one if the user has multiple on the same level. Lets say a contact doesn't have a local account but has two cloud ids in the address book. Which one should be shown then. We can't just use a random one there.

@juliusknorr juliusknorr force-pushed the enhancement/noid/sharing-consolidation branch from 8350f49 to 8a57415 Compare October 23, 2018 07:26
@skjnldsv skjnldsv dismissed their stale review October 23, 2018 12:17

Obsolete

@jancborchardt
Copy link
Member

@juliushaertl hm, as far as talking with @MorrisJobke & @rullzer I thought the main point of the consolidation was to actually consolidate people into one entry.

Lets say a contact doesn't have a local account but has two cloud ids in the address book. Which one should be shown then. We can't just use a random one there.

Why not share it to both in the background, and just see where they accept? We don’t really know which cloud they use anyway. Ideally then afterwards we know what they accepted with and that will be set as the standard method.

Yes, grouping the person and removing the duplicate avatars are good first steps – but the initial plan was to really make clear that it’s one and the same person, and that you as the person who shares really shouldn’t bother which way you share → it’s only important to share with the person.

@juliusknorr
Copy link
Member Author

juliusknorr commented Oct 24, 2018

hm, as far as talking with @MorrisJobke & @rullzer I thought the main point of the consolidation was to actually consolidate people into one entry.

Ok, I just couldn't find any issue discussion about it, that was why I asked for more input. 👍

Why not share it to both in the background, and just see where they accept? We don’t really know which cloud they use anyway. Ideally then afterwards we know what they accepted with and that will be set as the standard method.

That is probably something we can consider for later, since that would require us to store the preffered share method somewhere and also have a way in the sharing backend to not show up multiple shares in the ui then. Seems a bit to much for 15 to me 😉

Yes, grouping the person and removing the duplicate avatars are good first steps – but the initial plan was to really make clear that it’s one and the same person, and that you as the person who shares really shouldn’t bother which way you share → it’s only important to share with the person.

How about picking a default share option for contacts while searching, but once we have a exact match of the search string (e.g the user has entered the complete name) we show the other share options as well. That way you still have the choice if you want to, but in the regular case the user can quickly share using the best available method.

Example:

User types "Jan"
Share dialog shows:

  • Jan-Christoph Borchardt (will share with the first we have of local user/federation/email)

User types "Jan-Christoph Borchardt"
Share dialog shows:

  • Jan-Christoph Borchardt (local user)
  • Jan-Christoph Borchardt (federation)
  • Jan-Christoph Borchardt (email)

Edit: To make it a bit easier we maybe could allow to use the tab-key to complete the display name of the currently selected entry.

  • User types "Jan"
  • Share dialog shows entry: Jan-Christoph Borchardt
  • User hits tab
  • Share input field is filled with Jan-Christoph Borchardt
  • All share options show up as descriped above

@juliusknorr juliusknorr force-pushed the enhancement/noid/sharing-consolidation branch from 57d6c07 to 4d40a18 Compare October 26, 2018 11:47
@juliusknorr
Copy link
Member Author

@nextcloud/designers Here is how the solution I've described above looks like:

Fuzzy search with consolidated results:
image

Hit tab to complete the currently selected contact name, all contact methods are shown:
image

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
…ouping

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

Ready to review.

CI failure is a timeout: https://drone.nextcloud.com/nextcloud/server/12057/192

@jancborchardt
Copy link
Member

How about picking a default share option for contacts while searching, but once we have a exact match of the search string (e.g the user has entered the complete name) we show the other share options as well. That way you still have the choice if you want to, but in the regular case the user can quickly share using the best available method.

Wow, great solution @juliushaertl! 🎉 :) 👍

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Looking really great! Amazing work!! 🎉
Small changes though? :)

apps/dav/lib/CardDAV/AddressBookImpl.php Show resolved Hide resolved
core/css/jquery-ui-fixes.scss Show resolved Hide resolved
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

@skjnldsv Comments addressed.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Awesome stuff looking good!

@rullzer
Copy link
Member

rullzer commented Oct 30, 2018

Only minor comment (but we can fix that later)

Is that the default dropdown is quite small on firefox for me

drop

But as said lets get this in and fix minor stuff in a later PR so this doesn't get even more huge

@rullzer rullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 30, 2018
@juliusknorr
Copy link
Member Author

Is that the default dropdown is quite small on firefox for me

Do you have the gallery app installed? Iirc there was a style overwriting the server rules, I can have a look later.

@rullzer rullzer merged commit 41ff001 into master Oct 30, 2018
@rullzer rullzer deleted the enhancement/noid/sharing-consolidation branch October 30, 2018 18:27
@rullzer
Copy link
Member

rullzer commented Oct 30, 2018

Yes I had this enabled on my tests.

@jancborchardt
Copy link
Member

Do you have the gallery app installed? Iirc there was a style overwriting the server rules, I can have a look later.

O lala, another reason to get rid of the legacy code :O

@jospoortvliet
Copy link
Member

jospoortvliet commented Nov 1, 2018

Is there anything the gallery does now we have grid view? Ok, it is more focused on images etc but I'd separate the actual viewing view and have that in core/Files and have the gallery as an optional app users can install. Few will, I bet.

More on-topic, I'll test this a bit once it is in the daily builds ;-)

@MorrisJobke
Copy link
Member

Is there anything the gallery does now we have grid view?

Yes - slideshow is still in the gallery.

@jospoortvliet
Copy link
Member

Yeah, I thought so... Ok, for later then.

@jancborchardt
Copy link
Member

I'd separate the actual viewing view and have that in core/Files and have the gallery as an optional app users can install. Few will, I bet.

Yup, the next step is to write a new universal viewer. Spec at #12382
Afterwards we should indeed stop shipping Gallery as it duplicates functionality of grid view, and is pretty outdated and unmaintained. Grid view is a better and well-integrated replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants