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

[material-ui][Avatar] Stop insisting on using an icon for an empty avatar #41291

Open
Studio384 opened this issue Feb 27, 2024 · 16 comments
Open
Assignees
Labels
component: avatar This is the name of the generic UI component, not the React module! discussion package: material-ui Specific to @mui/material

Comments

@Studio384
Copy link
Contributor

Studio384 commented Feb 27, 2024

Steps to reproduce

Link to live example: https://mui.com/material-ui/react-avatar/

Steps:

  1. Have any Avatar component without any content.

Current behavior

In Material UI v5.15.7, a change was made that forces any Avatar component that does not have content to fallback to the Material Design "person" icon. This seems to me like a breaking change, and incredibly annoying behavior at that too since there is no way to disable this.

Expected behavior

When I define an Avatar without content, I expect it to not have content.

Context

No response

Your environment

No response

Search keywords: avatar fallback

@Studio384 Studio384 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 27, 2024
@danilo-leal danilo-leal changed the title [Material][Avatar] Stop insisting on using an icon for an empty avatar [material-ui][Avatar] Stop insisting on using an icon for an empty avatar Feb 27, 2024
@danilo-leal danilo-leal added component: avatar This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Feb 27, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Feb 28, 2024

For workaround, you can do:

<Avatar src="/broken-image.jpg" children=" " />

I'm not sure at this point what's the best experience for this but it's something we could consider in v7. cc @DiegoAndai

@siriwatknp siriwatknp added support: question Community support but can be turned into an improvement v7.x discussion and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer support: question Community support but can be turned into an improvement labels Feb 28, 2024
@DiegoAndai
Copy link
Member

I added it to the v7 milestone.

@zanivan, what do you think about this issue? What should be the fallback for the Avatar component?

@DiegoAndai DiegoAndai added this to the Material UI: v7 (draft) milestone Mar 5, 2024
@zanivan
Copy link
Contributor

zanivan commented Mar 6, 2024

what do you think about this issue? What should be the fallback for the Avatar component?

Although I like the way it's currently handled (first fallback is the first letter of the alt text, and the second is a generic avatar icon) I believe the best would be to render nothing at all instead of a generic avatar icon, and have something like <AvatarFallback /> where the user can set their default for this case—and Material UI's default <AvatarFallback /> could be something like <Paper />.

@Studio384
Copy link
Contributor Author

Is restoring the behavior the component has had and against which developers have built for years prior to v5.15.7 really something that can be postponed for 2 major releases? In what way is this behavioral change in v5.15.7 not a breaking change?

@DiegoAndai
Copy link
Member

DiegoAndai commented Mar 6, 2024

For context, the PR in which this was introduced: #40766

Hey @Studio384

Is restoring the behavior the component has had and against which developers have built for years prior to v5.15.7 really something that can be postponed for 2 major releases?

I'm not sure we will restore the behavior. This is added for v7 because we will perform design updates, and with that, we might want to change the fallback, regardless of whether an empty string or boolean child shows said fallback.

I'm sorry that the behavior you relied on changed, and I understand it can be frustrating. With that said, the new behavior is more consistent with the documentation stating that the last fallback is the empty avatar icon. This makes the component more predictable, as there's no documentation about showing no content at all.

I would gladly help you achieve what you're looking for in a documented way that stays intact with patch updates. Could you describe what you are trying to achieve? Is the end goal an empty avatar, or is there something else? Did you try this workaround?

Again, sorry for the inconvenience this might have caused and I hope we can help you.

@Studio384
Copy link
Contributor Author

Studio384 commented Mar 6, 2024

I have implemented the workaround. It's just wild that it is necessary to do at all, especially given that the workaround itself actively implies that what you're doing is wrong and not how that component should be used, and that there won't be an actual proper fix for this until 2 major versions later.

If the component's API acted differently than documented as it did in this instance, I'd argue that should mean that the documentation is what should be changed to reflect the discrepancy in behavior rather than introducing a breaking change into the API. The way this was fixed just seems backwards, instead of just updating the description in the documentation, why choose to break any application that may have relied on this behavior. If this behavior was clearly a bug, I'd understand, but I'd argue that most people would expect a simple component like an avatar without a child to not render a child, as the Avatar component has always done, disregarding knowledge of that one section of the documentation, nobody would call the original behavior a bug.

Especially since this seems to clash with previous instances where this project seems to have remained keen - and rightfully so - on not making any breaking changes in minor and patch updates, even if that proposed behavior made more sense or was the original intent.

At the very least, why not add an option to disable the fallback to v5?

@zanivan
Copy link
Contributor

zanivan commented Mar 6, 2024

At the very least, why not add an option to disable the fallback to v5?

I don't see a problem with this, actually. This way it'd be an opt-in feature, and IMO wouldn't be a breaking change. Could be something like <Avatar disableFallback /> or something—and then we can properly update the docs to add this. @DiegoAndai does it make sense?

@siriwatknp
Copy link
Member

At the very least, why not add an option to disable the fallback to v5?

I don't see a problem with this, actually. This way it'd be an opt-in feature, and IMO wouldn't be a breaking change. Could be something like <Avatar disableFallback /> or something—and then we can properly update the docs to add this. @DiegoAndai does it make sense?

I don't mind the disableFallback prop.

@DiegoAndai
Copy link
Member

This way it'd be an opt-in feature, and IMO wouldn't be a breaking change. Could be something like or something—and then we can properly update the docs to add this.

I think it might be a good idea. I have some questions, though:

  • What would we render when the fallback is disabled? The solid color?
  • Would disableFallback disable all fallbacks (children, alt first letter, user icon)? Or only the user icon one?
  • Is there a better API design for this? I wouldn't like to add a prop that we remove in v7 for a better implementation.
  • I would like to know (maybe not for now but for v7) what % of users prefer which fallback. Is this a common enough use case that it requires a prop?

@zanivan
Copy link
Contributor

zanivan commented Mar 7, 2024

  • What would we render when the fallback is disabled? The solid color?
    I think we could render a surface, using <Paper/>

  • Would disableFallback disable all fallbacks (children, alt first letter, user icon)? Or only the user icon one?
    For me, it should disable all fallbacks, since it's boolean. This would avoid confusion, because it's either with fallback or no fallback at all.

  • Is there a better API design for this? I wouldn't like to add a prop that we remove in v7 for a better implementation.
    Maybe there is, I'll drop some examples: Radix offers <Avatar.Fallback/> to define what will be the fallback, Vercel in their DS offers placeholder prop to render nothing, Ark also offers <Avatar.Fallback/>, Medusa UI offers fallback prop as a string, Reshaped renders a solid color. On the other hand, Mantine and Chackra works similarly to Material UI.

  • I would like to know (maybe not for now but for v7) what % of users prefer which fallback. Is this a common enough use case that it requires a prop?
    I don't know for certain, maybe we could mark this as "waiting for 👍" just to gauge the interest?

@DiegoAndai
Copy link
Member

DiegoAndai commented Mar 8, 2024

I added the "waiting for 👍🏼" label.

I think we could render a surface, using <Paper/>

What would you say are the benefits of using Paper?

@zanivan
Copy link
Contributor

zanivan commented Mar 8, 2024

What would you say are the benefits of using Paper?

I think it's mainly because it's a plain surface, so it'd solve the problem with the icon being too opinionated and specific for a use case where the displays a person. At the same time, Paper has the background.paper, so it wouldn't be an invisible component when empty.

@Studio384
Copy link
Contributor Author

What would you say are the benefits of using Paper?

I think it's mainly because it's a plain surface, so it'd solve the problem with the icon being too opinionated and specific for a use case where the displays a person. At the same time, Paper has the background.paper, so it wouldn't be an invisible component when empty.

An Avatar already isn't invisible when completely empty. My request is for it to not have any children, to just go back how Avatars have worked before v5.15.7, just give a component that shows a circle and has some presets for font sizes, etc. when something is entered. I feel like increasing complexity by adding a Paper when empty is unnecessary and just replaces one fallback for another.

@DiegoAndai
Copy link
Member

This makes sense to me:

  • disableFallback prop for disabling all fallbacks
  • If disabledFallback = true, then we render the empty solid color circle

But I'm not convinced that this is a common enough use case that we should add a prop for.

The more I think about it, the more the previous API (changed in #40766) seems like a good, simpler alternative. When the children prop is "", it makes sense to expect the empty solid-color circle, as the documentation states,

If there is an error loading the avatar image, the component falls back to an alternative in the following order:

  • the provided children
  • the first letter of the alt text
  • a generic avatar icon

This shows that these API decisions are not trivial and should be taken carefully. I'm leaving a comment in the PR that changed the API so that people involved can share their opinions as well.

@mirus-ua
Copy link
Contributor

Thank you, Diego, that pinged me.

IMO, the PR fixed the behavior according to the existing documentation. As for me, it is hard to say that falsy value should be considered as a wanted children prop.

Maybe we should keep the change BUT allow to customize the icon itself, because the logic, again for me, looks solid, but maybe someone wants to change the icon for a branded one or remove it at all.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Mar 12, 2024

I don't have any strong opinions, but I'm not in favor of adding the disableFallback prop. Disabling the fallback essentially reverts to not addressing #16161 even though it's a optional prop. Instead, I suggest adding a fallbackIcon slot for customizing the fallback icon. If developers prefer not to render anything as in this issue, they can provide null to fallbackIcon.

We received a previous request to change the fallback icon in issue #33229. Addressing this request would also resolve both issues. Additionally, it's unclear why specifically the Person icon was chosen as the last fallback in #18711.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: avatar This is the name of the generic UI component, not the React module! discussion package: material-ui Specific to @mui/material
Projects
None yet
Development

No branches or pull requests

8 participants