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

Add avatar component with contacts menu #64

Merged
merged 4 commits into from
Oct 26, 2018
Merged

Add avatar component with contacts menu #64

merged 4 commits into from
Oct 26, 2018

Conversation

juliusknorr
Copy link
Contributor

@juliusknorr juliusknorr commented Oct 12, 2018

Fixes #14

Example usage:

<!-- own avatar -->
<avatar user="admin" displayName="Julius" />

<!-- other users avatar -->
<avatar user="anexistinguser" displayName="An existing user" />

<!-- user to avatar image -->
<avatar :url="url" />

<!-- non-existing user -->
<avatar user="nonexistinguser" />

image

@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 12, 2018
src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved
src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved
src/components/PopoverMenu/PopoverMenuItem.vue Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Contributor

While trying to test this in Mail I found a use case that is not yet supported: non-Nextcloud user but without an URL.

We don't always have a URL to an avatar. In those cases we still want to render an avatar with the sender's email address or name. This should more or less assemble what I've developed at https://github.com/nextcloud/mail/blob/b5ad1a0ddf3ca03d0b43e7c0fa835f4607279b34/js/components/Avatar.vue (simple placeholder with first letter).

@ChristophWurst
Copy link
Contributor

ChristophWurst commented Oct 15, 2018

  • Bug/limitation?: user of the component has to always specify the displayName in order to get a tooltip. If only the UID is available that won't be possible. Should we just use the user prop as fallback in that case?

@ChristophWurst

This comment has been minimized.

@juliusknorr
Copy link
Contributor Author

We don't always have a URL to an avatar. In those cases we still want to render an avatar with the sender's email address or name. This should more or less assemble what I've developed at nextcloud/mail:js/components/Avatar.vue@b5ad1a0 (simple placeholder with first letter).

Yep, let's include that as well. I'll look into that.

@juliusknorr
Copy link
Contributor Author

user of the component has to always specify the displayName in order to get a tooltip. If only the UID is available that won't be possible. Should we just use the user prop as fallback in that case?

I don't see a big use case in adding that, also showing the user id might not be very useful on ldap setups for example. If the displayname is not available app devs can still just set the displayname to the same value as the user id to get a tooltip with it.

Besides that, all the comments should be addressed. Ready for another round of review. 😉

src/components/Avatar/Avatar.vue Show resolved Hide resolved
src/components/Avatar/Avatar.vue Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Contributor

Any solution for #64 (comment)?

I've (again) integrated this new component in Mail: nextcloud/mail#1224.

Right now it looks like this:
bildschirmfoto von 2018-10-24 13-19-17

What's missing from my POV:

Works great otherwise ✌️

@ChristophWurst
Copy link
Contributor

Would it make sense to show a placeholder for unknown users with known displayName and :allowPlaceholder="true"? I see you already took the $.imageplaceholder logic from server (which I used at https://github.com/nextcloud/mail/blob/c1c875db07b59f500dd5ed695f772d8f979238f9/src/components/Avatar.vue#L18) to build the placeholder for known users. This should cover my use case from #64 (comment).

@juliusknorr
Copy link
Contributor Author

juliusknorr commented Oct 24, 2018

Would it make sense to show a placeholder for unknown users with known displayName and :allowPlaceholder="true"? I see you already took the $.imageplaceholder logic from server (which I used at nextcloud/mail:src/components/Avatar.vue@c1c875d#L18) to build the placeholder for known users. This should cover my use case from #64 (comment).

This should already work with :user and :allowPlaceholder="true". Is there a specific reason you might want to use displayname only instead of a user id? Ah right, because using :user will always try to fetch the avatar first, which doesn't make sense for the mail app 😉 I'll add that as well.

@ChristophWurst
Copy link
Contributor

Ah right, because using :user will always try to fetch the avatar first, which doesn't make sense for the mail app wink I'll add that as well.

Exactly. Thanks ✌️

@juliusknorr
Copy link
Contributor Author

@ChristophWurst

#64 (comment) I'd like to show a placeholder instead

<avatar displayName="Example" /> will do that now, as discussed.

The possibility to prevent the tooltip as it doesn't add much if the display name is already displayed next to the avatar

Combined with the above, you can also disable the tooltip with:

<avatar displayName="FooBar" disableTooltip="true" />

@skjnldsv I've tried to be a bit more descriptive with the variable names.

@juliusknorr
Copy link
Contributor Author

CI is also failing on master :/ https://travis-ci.com/nextcloud/vue-components/builds/89187769

@ChristophWurst
Copy link
Contributor

Thanks a lot 🚀 🎊 🌮

bildschirmfoto von 2018-10-25 18-46-43
✔️ works with display names only 🚀

bildschirmfoto von 2018-10-25 18-46-28
✔️ works with custom URLs 🚀

Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Awesome! Tested it at nextcloud/mail#1224 and it works really well.

@skjnldsv skjnldsv added the feature: avatar Related to the avatar component label Oct 26, 2018
examples/index.html Outdated Show resolved Hide resolved
src/components/PopoverMenuButton/PopoverMenuButton.vue Outdated Show resolved Hide resolved
@skjnldsv skjnldsv merged commit 890bfc4 into master Oct 26, 2018
@skjnldsv skjnldsv deleted the avatar branch October 26, 2018 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews feature: avatar Related to the avatar component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants