Skip to content

Show transparent contact photo on a lighter background#1415

Closed
hanzi wants to merge 1 commit into
nextcloud:masterfrom
hanzi:transparent-photo-in-list
Closed

Show transparent contact photo on a lighter background#1415
hanzi wants to merge 1 commit into
nextcloud:masterfrom
hanzi:transparent-photo-in-list

Conversation

@hanzi
Copy link
Copy Markdown
Member

@hanzi hanzi commented Jan 17, 2020

fixes #1414
fixes #1066

This improves support for transparent contact photos by

  1. Removing the still visible first letter in the contact list
  2. Placing photos on a much lighter background colour so it doesn't clash with the transparent image

SharedScreenshot

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 17, 2020

Codecov Report

Merging #1415 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1415   +/-   ##
=======================================
  Coverage   62.31%   62.31%           
=======================================
  Files           4        4           
  Lines          69       69           
=======================================
  Hits           43       43           
  Misses         26       26

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 503be80...efb8aa8. Read the comment docs.

@skjnldsv skjnldsv added 3. to review Waiting for reviews bug Something isn't working design Related to the design labels Jan 18, 2020
@skjnldsv skjnldsv added this to the next milestone Jan 18, 2020
@jancborchardt
Copy link
Copy Markdown
Member

Very cool @hanzi! Would this also fix it for the Mail app, as we have the same issue there: nextcloud/mail#1821

Possibly should be in the Vue component as well? @juliushaertl @skjnldsv @ChristophWurst, and cc @gary-kim as you were active in the issue too

<div class="contact-header-avatar__background" @click="toggleModal" />
<div v-if="contact.photo"
:style="{ 'backgroundImage': `url(${contact.photoUrl})` }"
:style="{ 'backgroundImage': `url(${contact.photoUrl})`, 'backgroundColor': 'rgba(255, 255, 255, 0.8)' }"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just use #fff here, since it looks weird with the color still shining through. :) Also we don’t need to theme it because with dark mode, the backgrounds of these images should still be white.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've changed it now, though I liked the old style a bit better. Made the contrast seem less harsh.

SharedScreenshot

@hanzi
Copy link
Copy Markdown
Member Author

hanzi commented Jan 18, 2020

@jancborchardt On second thought, you were probably referring to the contacts list and not the title bar of the details area (which is the code you've annotated.)

I've given this some thought beforehand: Making the background transparent/white at the very least makes the list look less structured. But it starts looking really weird and confusing when an irregularly shaped avatar gets cropped in a circle shape: (see Santa's photo)

SharedScreenshot

So as an alternative, I tried setting it to lightgrey:

SharedScreenshot2

But to me, this also looks a bit dull, so the current version was actually my favourite:

SharedScreenshot3

@hanzi
Copy link
Copy Markdown
Member Author

hanzi commented Jan 18, 2020

Here's another one, using #eee as a background (which is the same as the 'selected contact' background colour in the default theme).

SharedScreenshot

@jancborchardt
Copy link
Copy Markdown
Member

jancborchardt commented Jan 19, 2020

This screenshot with white background actually looks best by far. :) It does not interfere with the icons and avatars, especially not with the color generated from our placeholder, which does not have anything to do with the actual avatars. 👍
(And yes, white #fff background for both contact list and contact detail view, and also for dark theme because otherwise logos/icons will not be visible.)

SharedScreenshot

@hanzi
Copy link
Copy Markdown
Member Author

hanzi commented Jan 20, 2020

I still prefer the last one I posted, with the very light grey background.

Reason being (as I said before) that this way it feels less 'broken' when a transparent image (such as Santa's hat in my example) gets cropped. Because it's such an irregular shape, you don't really see that it's just cropped into a circle. Just seems like parts of the picture are missing for no good reason.

With the light grey background you can always see the full circle so the cropping doesn't feel as surprising to me.

But if you still want the white version, that's your call to make. Just let me know and I'll fix it.

@jancborchardt
Copy link
Copy Markdown
Member

Right, so for that we could do it like Twitter – they seem to use a veeery very slight box-shadow: inset on the avatars to show a super thin border around the avatar.

If we do that, we tick all the boxes:

  • Doesn't change the background of pictures but keeps it white
  • Makes clear that pictures are circular and cropped
  • Looks nice

What do you think? Best check at Twitter and inspect it there, or let me know and I can do it. :)

@hanzi hanzi force-pushed the transparent-photo-in-list branch from b0f97c1 to 199ae69 Compare January 20, 2020 22:27
@hanzi
Copy link
Copy Markdown
Member Author

hanzi commented Jan 20, 2020

I couldn't find any place on Twitter where they use an inset box shadow, but I like the idea.

SharedScreenshot

How about it?

@hanzi hanzi force-pushed the transparent-photo-in-list branch from 199ae69 to e8f01f3 Compare January 20, 2020 22:29
@hanzi
Copy link
Copy Markdown
Member Author

hanzi commented Jan 24, 2020

@jancborchardt What do you think about the version with the very faint inset shadow?

Once we've decided on one, I would like to apply the same solution to the Avatar component on nextcloud/vue which should fix it in Mail as well.

Signed-off-by: Christian Kraus <hanzi@hanzi.cc>
@hanzi hanzi force-pushed the transparent-photo-in-list branch from e8f01f3 to efb8aa8 Compare January 24, 2020 11:26
@jancborchardt
Copy link
Copy Markdown
Member

@hanzi sorry I haven’t had time to test it due to the time it takes updating the whole local instance and being on my way to vacation. :\ From looking at it it seems fine, but could be subdued a bit – less intense.

You can check the value Twitter uses on a profile with a transparent avatar, like https://twitter.com/osdiversity

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 28, 2020
backgroundColor: '#fff',
// The contact photo gets cropped in a circular shape, which might look odd with transparent photos.
// This box shadow ensures that there's always a very faint edge hinting at the circle.
boxShadow: '0 0 6px rgba(0, 0, 0, 0.08) inset',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
boxShadow: '0 0 6px rgba(0, 0, 0, 0.08) inset',
boxShadow: '0 0 5px rgba(0, 0, 0, 0.05) inset',

Let’s go for a bit more subdued here. :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For reference, this is what Twitter uses and looks quite nice:

box-shadow: rgba(0, 0, 0, 0.02) 0px 0px 2px inset;
border: 1px solid rgba(0, 0, 0, 0.04);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, we have a variable for this, no?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, as this is very new. Also, in dark mode it can be exactly the same as it’s just very transparent black.

@jancborchardt
Copy link
Copy Markdown
Member

Hi @hanzi, I’m back from vacation now. Any update on this one, did you check out the values Twitter uses? In any case I think making it a bit more subdued is better – I posted a suggestion in the code, what do you think?

Also cc @skjnldsv your input?

@skjnldsv
Copy link
Copy Markdown
Member

Hey! So what is the status here? :)

@jancborchardt
Copy link
Copy Markdown
Member

Hey! So what is the status here? :)

I’d say let’s merge this, I will submit a follow-up pull request to fix the details? :)

<div class="contact-header-avatar__background" @click="toggleModal" />
<div v-if="contact.photo"
:style="{ 'backgroundImage': `url(${contact.photoUrl})` }"
:style="{ 'backgroundImage': `url(${contact.photoUrl})`, 'backgroundColor': '#fff' }"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shall we use default background variable here @jancborchardt ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, as said above at #1415 (comment)

Just use #fff here, since it looks weird with the color still shining through. :) Also we don’t need to theme it because with dark mode, the backgrounds of these images should still be white.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oups, sorry! Missed this! 🙈

Copy link
Copy Markdown
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.

Code looks good to me! 👍
Thanks!

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 21, 2020
@skjnldsv skjnldsv requested a review from jancborchardt April 21, 2020 16:26
@skjnldsv
Copy link
Copy Markdown
Member

@hanzi can you rebase? :)
@jancborchardt, final review?

@jancborchardt
Copy link
Copy Markdown
Member

Not sure @hanzi will reply since they have not been active since January. I’ll create a pull request based on this and doing the adjustments I mentioned in the comments, and rebased. :)

@jancborchardt
Copy link
Copy Markdown
Member

Rebased and with my added small adjustment at #1596, let’s continue there :)

@hanzi
Copy link
Copy Markdown
Member Author

hanzi commented Apr 27, 2020

Hi! Sorry for my prolonged absence, got very busy with work and had to tune out all other distractions for a while. 😃

Thanks @jancborchardt for picking it up and getting it merged!

@hanzi hanzi deleted the transparent-photo-in-list branch April 27, 2020 07:55
@jancborchardt
Copy link
Copy Markdown
Member

No worries at all @hanzi, very cool collaboration! :) If you want to pick up something new, there’s plenty of other issues. :D

(Also, if you are interested, we can invite you to our Contacts team chat?)

@skjnldsv skjnldsv modified the milestones: next, 3.4.0 Oct 1, 2020
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 bug Something isn't working design Related to the design

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mismatch displaying PNG-images in Nextcloud-Contacts Avatar placeholder shows behind partly transparent avatar

3 participants