Coder Social home page Coder Social logo

Comments (16)

ZeeshanTamboli avatar ZeeshanTamboli commented on April 28, 2024 4

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.

from material-ui.

DiegoAndai avatar DiegoAndai commented on April 28, 2024 1

I added it to the v7 milestone.

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

from material-ui.

zanivan avatar zanivan commented on April 28, 2024 1

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 />.

from material-ui.

Studio384 avatar Studio384 commented on April 28, 2024 1

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?

from material-ui.

siriwatknp avatar siriwatknp commented on April 28, 2024 1

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.

from material-ui.

DiegoAndai avatar DiegoAndai commented on April 28, 2024 1

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?

from material-ui.

DiegoAndai avatar DiegoAndai commented on April 28, 2024 1

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?

from material-ui.

DiegoAndai avatar DiegoAndai commented on April 28, 2024 1

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.

from material-ui.

siriwatknp avatar siriwatknp commented on April 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

from material-ui.

Studio384 avatar Studio384 commented on April 28, 2024

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?

from material-ui.

DiegoAndai avatar DiegoAndai commented on April 28, 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.

from material-ui.

zanivan avatar zanivan commented on April 28, 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?

from material-ui.

zanivan avatar zanivan commented on April 28, 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?

from material-ui.

zanivan avatar zanivan commented on April 28, 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.

from material-ui.

Studio384 avatar Studio384 commented on April 28, 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.

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.

from material-ui.

mirus-ua avatar mirus-ua commented on April 28, 2024

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.

from material-ui.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.