[Avatar] Use inner shadow for border #4261

mbrookes · 2016-05-13T21:03:41Z

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Use an inner shadow for the border to allow the image to be an <img> not backgroundImage.

Closes #4254.

nathanmarks · 2016-05-13T22:26:10Z

src/Avatar/Avatar.js

-      border: `solid 1px ${avatar.borderColor}`,
-      height: size - 2,
-      width: size - 2,
+      boxShadow: `inset 0px 0px 0px 1px ${avatar.borderColor}`,


Since it's just a single property, we may as well assign it directly instead of using Object.assign()

Good point. I know that looked wrong. :)

nathanmarks · 2016-05-13T22:26:41Z

@mbrookes one tiny change, looks good otherwise

mbrookes · 2016-05-13T22:36:32Z

On hold for a 0.16.0 branch or pre-release.

oliviertassinari · 2016-05-14T09:11:14Z

src/Avatar/Avatar.js

@@ -22,6 +22,7 @@ function getStyles(props, context) {
      borderRadius: '50%',
      height: size,
      width: size,
+      boxShadow: src ? `inset 0px 0px 0px 1px ${avatar.borderColor}` : 'none',


I have tried this path in the past. But we ended up reverting this change for painting performances reasons #1204 (comment).
I'm not sure it worth it.

Oh, interesting, I compared the current and new implementation, and couldn't see a meaningful difference, but perhaps I'm not testing correctly.

Are you sure about the visual impact of this property? It seems to me that it has no impact and that backgroundColor seems to be relevant here (I have no idea why).

The impact of boxShadow?

yes, the visual impact. I can't see any difference with or without the property on Chome v50 OSX.

nathanmarks · 2016-05-14T18:09:52Z

@oliviertassinari what about a border on a pseudo element? What's the perf like?

I think being able to use an img as an avatar is an important feature.

nathanmarks · 2016-05-14T18:11:13Z

@oliviertassinari FWIW Google don't use the border in any of their products from android contacts book to desktop inbox app. I don't mind keeping it (it looks good on white avatars etc) but I'd like to see a solution that lets us use an IMG tag

oliviertassinari · 2016-05-14T18:28:59Z

@nathanmarks I definitely agree with you. Using an <img /> would be much better semently speaking. We should aim for this 👍 .
I haven't benchmarked the rendering (painting) time needed for the box-shadow yet. I should do it.
I see three options:

We keep this PR like this
We add an extra div to get the desired border opacity
We remove the border

Either way, we should consider performances vs the spec perfect implementation ✨.

nathanmarks · 2016-05-14T18:33:35Z

@oliviertassinari BTW, the border is not even detailed in the spec -- it's just visually present in the examples (perhaps why it isn't in any of the products)

oliviertassinari · 2016-05-17T18:57:51Z

After some quick benchmark with 30+ displayed Avatar, I couldn't notice any significant increase of the painting time.

mbrookes · 2016-05-17T21:28:09Z

@oliviertassinari, you're right, there's something odd going on here. The backgroundColor does seem to be bleeding out ever so slightly from behind the image (barely a single physical pixel on a retinal display.)

Also, from a bit of googling, it seems a boxshadow over an image doesn't work 'out of the box' (hoho).

However under some circumstances, boxShadow does appear to do something on an image Avatar... It's subtle, but take a look over the guy's left shoulder, and along the top of the circle:

Unless I'm imagining this, there a difference, which is what led me to believe this was working.

However, when changing the border size to something more visible, it's clear that this isn't actually working as expected as the border doesn't get bigger, and even though it sort of works here, it's unlikely to be consistent across browsers, so we need a different solution...

nathanmarks · 2016-05-17T22:37:52Z

@mbrookes I usually have to use a pseudo element to achieve it.

nathanmarks · 2016-05-17T22:38:47Z

@mbrookes at which point, for this use case, may as well have a border. But I use shadows over images often for vignetting etc

mbrookes · 2016-05-17T23:15:30Z

Okay, I went the overlay div route. 😢

I did try a border on the overlay (with boxModel: 'border-box'), but it didn't perfectly overlay the image when zoomed in. That might be something else I've done wrong though.

mbrookes · 2016-05-17T23:17:11Z

I'll fix the test after we agree on the approach.

oliviertassinari · 2016-05-18T20:18:41Z

I'll fix the test after we agree on the approach.

As pointed out by @nathanmarks Google isn't adding a border on their apps. On my project, I do neither. I would go with option 3 and remove the border.
What do you guys think?

nathanmarks · 2016-05-18T20:27:04Z

@oliviertassinari That was my gut instinct 👍

mbrookes · 2016-05-18T20:42:45Z

Avatar images with a white background against a white background look pretty awful (@nathanmarks posted a good example in chat), but perhaps they're not common enough to worry about.

I think the border overlay is the right design choice, but the code (and possible performance) overhead might not be worth it from an engineering standpoint.

nathanmarks · 2016-05-18T20:53:41Z

@mbrookes No borders lets you do this (note the non-round avatars here)

nathanmarks · 2016-05-18T20:58:44Z

@mbrookes @oliviertassinari

I found borders in the wild!!

nathanmarks · 2016-05-18T21:00:06Z

IMO, I think borderless actually looks better 99.9% of the time in that application due to the technique.

mbrookes · 2016-05-27T12:14:54Z

Closing in favour of #4265.

mbrookes added the PR: Needs Review label May 13, 2016

nathanmarks added the breaking change label May 13, 2016

nathanmarks reviewed May 13, 2016
View reviewed changes

nathanmarks added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels May 13, 2016

[Avatar] Use inner shadow for border

1dcacbe

mbrookes added this to the 0.16.0 Release milestone May 13, 2016

mbrookes added the on hold There is a blocker, we need to wait label May 13, 2016

nathanmarks added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 13, 2016

oliviertassinari reviewed May 14, 2016
View reviewed changes

mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels May 17, 2016

[Avatar] Use overlay div for border

112524a

mbrookes added the PR: Needs Review label May 18, 2016

nathanmarks removed on hold There is a blocker, we need to wait PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 21, 2016

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels May 23, 2016

mbrookes mentioned this pull request May 27, 2016

[Avatar] Remove the border #4365

Merged

3 tasks

mbrookes closed this May 27, 2016

mbrookes deleted the avatar-use-inner-shadow-for-border branch September 11, 2018 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Avatar] Use inner shadow for border #4261

[Avatar] Use inner shadow for border #4261

mbrookes commented May 13, 2016

nathanmarks May 13, 2016

mbrookes May 13, 2016

nathanmarks commented May 13, 2016

mbrookes commented May 13, 2016

oliviertassinari May 14, 2016 •

edited

Loading

mbrookes May 14, 2016

oliviertassinari May 17, 2016

mbrookes May 17, 2016

oliviertassinari May 17, 2016

nathanmarks commented May 14, 2016

nathanmarks commented May 14, 2016

oliviertassinari commented May 14, 2016 •

edited

Loading

nathanmarks commented May 14, 2016

oliviertassinari commented May 17, 2016

mbrookes commented May 17, 2016

nathanmarks commented May 17, 2016

nathanmarks commented May 17, 2016

mbrookes commented May 17, 2016

mbrookes commented May 17, 2016

oliviertassinari commented May 18, 2016 •

edited

Loading

nathanmarks commented May 18, 2016

mbrookes commented May 18, 2016

nathanmarks commented May 18, 2016 •

edited

Loading

nathanmarks commented May 18, 2016 •

edited

Loading

nathanmarks commented May 18, 2016 •

edited

Loading

mbrookes commented May 27, 2016

[Avatar] Use inner shadow for border #4261

[Avatar] Use inner shadow for border #4261

Conversation

mbrookes commented May 13, 2016

nathanmarks May 13, 2016

Choose a reason for hiding this comment

mbrookes May 13, 2016

Choose a reason for hiding this comment

nathanmarks commented May 13, 2016

mbrookes commented May 13, 2016

oliviertassinari May 14, 2016 • edited Loading

Choose a reason for hiding this comment

mbrookes May 14, 2016

Choose a reason for hiding this comment

oliviertassinari May 17, 2016

Choose a reason for hiding this comment

mbrookes May 17, 2016

Choose a reason for hiding this comment

oliviertassinari May 17, 2016

Choose a reason for hiding this comment

nathanmarks commented May 14, 2016

nathanmarks commented May 14, 2016

oliviertassinari commented May 14, 2016 • edited Loading

nathanmarks commented May 14, 2016

oliviertassinari commented May 17, 2016

mbrookes commented May 17, 2016

nathanmarks commented May 17, 2016

nathanmarks commented May 17, 2016

mbrookes commented May 17, 2016

mbrookes commented May 17, 2016

oliviertassinari commented May 18, 2016 • edited Loading

nathanmarks commented May 18, 2016

mbrookes commented May 18, 2016

nathanmarks commented May 18, 2016 • edited Loading

nathanmarks commented May 18, 2016 • edited Loading

nathanmarks commented May 18, 2016 • edited Loading

mbrookes commented May 27, 2016

oliviertassinari May 14, 2016 •

edited

Loading

oliviertassinari commented May 14, 2016 •

edited

Loading

oliviertassinari commented May 18, 2016 •

edited

Loading

nathanmarks commented May 18, 2016 •

edited

Loading

nathanmarks commented May 18, 2016 •

edited

Loading

nathanmarks commented May 18, 2016 •

edited

Loading