Material-ui: Avatar shows children and image

[ajdaniel]

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When I pass in children and a src prop to the Avatar component, both are rendered. This is a change in behaviour that was not expected, and was caused by #17693.

Expected Behavior 🤔

Steps to Reproduce 🕹

Provide children and a src to the Avatar component, like so:

<Avatar src={person.avatar} className={classes.avatarPerson}>
  {person.name.substr(0, 1)}
</Avatar>

Context 🔦

We use this to display user profile images, but fallback to the user name initials if the image doesn't exist.

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.2.1 |

[oliviertassinari]

@ajdaniel It sounds like you have to pick between one of the two approaches. Why do you mix both?

[ajdaniel]

That's how it worked before. The children prop became a fallback and I didn't have to code the logic of showing or hiding the children based on the image availability, it just did that automatically.

[oliviertassinari]

The children prop description:

Used to render icon or text elements inside the Avatar. src and alt props will not be used and no img will be rendered by default.
This can be an element, or just a string.

It seems that prior to #17693, the implementation behave the opposite to the documentation.

[ajdaniel]

Okay that's not a problem, I can't argue against undocumented features. I had a feeling this was the case too, but I wanted to raise the point.

Huh, perhaps we need to clarify the props in the component too.

Perhaps we can add it as a feature? Perhaps instead of children we could have a prop like fallbackRender which renders if the src or srcSet is empty?

[oliviertassinari]

I would propose that we 1. update the prop description to match reality. 2. I wouldn't be against a customization demo per @lcswillems use case. 3. a test case to it.

Regarding your fallback proposal, imgFallbackChildren?: boolean has potential. However, I'm wondering if it's not better, for their developer to have the logic visible in their render methods. @mbrookes what do you think?

[lcswillems]

@oliviertassinari I am not sure to understand what you expect from me?

[oliviertassinari]

Nothing, I was including you for context :)

[mbrookes]

imgFallbackChildren?: boolean

That sounds horrible! 🙀We should pick one behaviour and stick to it. If #17693 hadn't already been merged, I'd favour @ajdaniel's use case over @lcswillems', and suggest a customization demo for the latter instead.

Not sure what the best path forward is though, short of reverting #17693 and updating the documentation.

[lcswillems]

@mbrookes Why would you prefer @ajdaniel use case over mine?

[oliviertassinari]

@mbrookes What do you think of the behavior in the children prop description? It seems to be a 3rd viable option:

1. children < img (#17876)
2. children + img (#17693)
3. children > img (API docs)

[mbrookes]

@lcswillems Because displaying one type of content in an Avatar is the norm, whereas your requirement is more of a customisation.

@oliviertassinari Why would 3. be preferable? From my experience, text is always used as a fallback to an image, not the other way around.

[lcswillems]

The problem with children > img is that the avatar img behavior will have to be reimplemented.

If we do children + img, @ajdaniel just have to conditionally display its children if src is set, that is much easier.

I think @ajdaniel filled an issue just because it broke his code, not necessarily because it was the good use case.

[lcswillems]

@mbrookes Why not have a fallbackText property and let the children be used.

[oliviertassinari]

That sounds horrible! 🙀

@mbrookes Yeah, I agree.

In my opinion, the fallback logic should be on the userland. I think that people should be able to infer it from reading their code:

<Avatar src={person.avatar} className={classes.avatarPerson}>
  {person.avatar ? null : person.name.substr(0, 1)}
</Avatar>

Otherwise, it's not obvious what will happen if both information is present, or if one is missing.
I do agree that an avatar image is a piece of superior information to the initial letters in the most common use cases, but what if somebody wants to display both?

Now, I don't have a strong preference between 1. and 2. The avatar was designed to display the information, not to edit it. @lcswillems could write its own logic, at this point, it could be simpler to bypass the Avatar.

I would propose:

1. @mbrookes choose the preferred approach.
2. We add a test case about it.
3. We update the prop description to match reality.

[mbrookes]

<Avatar src={person.avatar} className={classes.avatarPerson}>
  {person.avatar ? null : person.name.substr(0, 1)}
</Avatar>

That doesn't seem unreasonable.

what if somebody wants to display both?

In that case should the children be displayed _over_ the image (rather than beside it)?

[oliviertassinari]

@mbrookes I don't know, the use case I had in mind is somebody that want to apply customizations. But maybe it's an edge case and more people would benefit from the fallback behavior.

[adeelibr]

<Avatar src={person.avatar} className={classes.avatarPerson}>
  {person.avatar ? null : person.name.substr(0, 1)}
</Avatar>

If this is up for taking can I work on it? :)

[mark-tate]

Just to clarify my understanding of this thread.

Was wondering if you see fallback, covering the un-happy path, where src does not load and an onError occurs ?

When using src (or srcSet), I saw children as a fallback for the onError condition.
Whilst you have the alt prop, it does not look so great in practice, with style, borderRadius: 50%

Also, worth mentioning one of your other issues. not un-related to this.
https://github.com/mui-org/material-ui/issues/16161
Not sure, how valid that ask is but worth mentioning in this context.

[oliviertassinari]

@slipmat In practice is it frequent for the image URL to be broken? I would be in favor to ignore this problem.
@mbrookes In which direction should we go?

[mark-tate]

Ant Design works that way, although that's not a reason to follow suit.

With MUI you can pass through onError via imgProps, so there are ways around it.
Could be one to cover via the docs, if needed.

[mbrookes]

The userland fallback approach isn't too onerous. My overriding concern is that #17694 is a breaking change for developers depending on the previous (if incorrectly documented) logic.

[oliviertassinari]

@mbrookes OK, I propose that we revert #17694

• The most frequent use case it to render an avatar or a children, not both at the same time.
• For @lcswillems use case, my recommendation is to use the component prop or a custom made component.
• We should add a test case and update the prop description.
• in the future, we can imagine a prop to detect image load error and automatically fallback to the children (but I really doubt it worth the bundle size increase).

My only fear would be that we break composition, that looking at:

<Avatar src={person.avatar} className={classes.avatarPerson}>
  {person.name.substr(0, 1)}
</Avatar>

It's unclear what happen.

[lcswillems]

component prop? I don't see how to use it in my case.

And if I have to create a custom made component, I will have to copy paste all the base logic to increment on it.

[oliviertassinari]

@lcswillems

const EditAvatar = props => (
  <div {...props}>
    {props.children}
    <div>Edit</div>
  </div>
)

const <Avatar src="sef" component={EditAvatar} />

[lcswillems]

Okay, I see. I think it is the good design choice.

[adeelibr]

@lcswillems

const EditAvatar = props => (
  <div {...props}>
    {props.children}
    <div>Edit</div>
  </div>
)

const <Avatar src="sef" component={EditAvatar} />

should this be added in the demo as an example, if yes can I pretty please work on this?

[oliviertassinari]

@adeelibr I would rather avoid it, thanks for proposing to help. @eps1lon is looking into how we can get rid of the component prop.