[Avatar] Fallback images when fails to load #18711
Conversation
|
@material-ui/core: parsed: +0.18% , gzip: +0.32% Details of bundle changes.Comparing: e8703bf...fa995f0
|
oliviertassinari
force-pushed
the
fallbackImages
branch
from
e216ec8
to
c5925ec
Compare
oliviertassinari
added
component: avatar
oliviertassinari
force-pushed
the
fallbackImages
branch
from
c5925ec
to
fa995f0
Compare
|
@netochaves Thank for looking at it. I have added a new demo:
|
|
oh nice. i was doing that (use the letter) explicitly in my code and thus having to watch for the image load error externally. |
|
Hi guys, while I appreciate that this might be useful in this narrow albit popular use case for avatars it's a visual breaking change. I used the avatar component to build a swatch colour picker which should have no content when a colour isn't selected and a tick when they are. Now I have person icons in every unselected colour as well as the selected swatch.
Shouldn't this be opt in so that people can either choose the fallback image or not have one at all? |
|
@andokai We haven't anticipated this possibility. Why are you using the Avatar for this use case? Would it be better to render a div with a border radius of 50%? |
|
I was using the avatar because in addition to the border radius it centres the check icon in the circle. Of course I can do this all manually using a div but the same could be said for every component. I also don't believe that the function is described correctly. It doesn't fall back when the image fails to load, it now defaults to the person icon unless you supply it with children or it successfully loads the image. It also claims to fall back to a generic avatar icon but it's only generic if the only thing you ever list is people. |
|
The |
|
For context, the Avatar component is optimized for displaying a representation of a person. @andokai What if we scope the display of the fallback icon when the image has failed to load and that we have no other information to fallback to? What about the following diff? diff --git a/packages/material-ui/src/Avatar/Avatar.js b/packages/material-ui/src/Avatar/Avatar.js
index 9505a20f6..f2147a917 100644
--- a/packages/material-ui/src/Avatar/Avatar.js
+++ b/packages/material-ui/src/Avatar/Avatar.js
@@ -62,12 +62,12 @@ function useLoaded({ src, srcSet }) {
const [loaded, setLoaded] = React.useState(false);
React.useEffect(() => {
+ setLoaded(false);
+
if (!src && !srcSet) {
return undefined;
}
- setLoaded(false);
-
let active = true;
const image = new Image();
image.src = src;
@@ -130,7 +130,7 @@ const Avatar = React.forwardRef(function Avatar(props, ref) {
children = childrenProp;
} else if (hasImg && alt) {
children = alt[0];
- } else {
+ } else if (loaded === 'error') {
children = <Person className={classes.fallback} />;
}@Jeff-G Interesting, we might want to add an opt-in prop to remove the skeleton background color. But maybe a style override is as simple? |
Yes, that is indeed. However, in my created component, an avatar image uses an image with a transparent background as an exception, and this change resulted in an unintended style. As a countermeasure, I wrote a class that applies |
If that's true then perhaps you want a different component for list items? One of the list examples uses avatars for folders. https://material-ui.com/components/lists/#folder-list
This solves the particular problem I have but falling back to a person icon still seems like it's only satisfying one particular use case. One of the application have built is essentially an asset management product so one of the lists is people so this implementation could help but there's also lists of vehicles and locations and so on. In all of those circumstances I will need to setup a fallback icon when no image exists. So do I use this behaviour where I introduce inconsistent implementation depending on they type of item shown in the list or do I just generalise my approach and provide a fallback avatar regardless of the item type? |
|
@andokai By optimized, I meant, as the primary use case it's used for. |
|
It isn't necessarily that images fail to load. For my app, I'm not in charge of the server API, and an avatar image is not a requirement (as in, there's not a default image that is sent if the user didn't configure one for themselves). Thus, in my case it is actually likely that there won't be one, because not everybody bothers to do that; I'll get the 404 from the server and an image onload error to handle. I intentionally leave one of my accounts without one in order to test my workaround of rendering the initial when the image isn't there. As of yet, I have not attempted to update my code to use this new feature - I will be doing a major rewriting to be more Hooks based and Function components (and remove the alt flux store i've been using), likely coordinating to when MUI 5.x comes out, so I'll address it then. |
|
An improvement to this is could be to use https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/decode Adding images to DOM previously was synchronous and the decoding (jpeg, png) of the image was actually blocking. I ran across this PR because I was searching for smooth transition between image loads. I would be nice if there was a transition between images or using spinner or something. Everything in this library looks awesome except this thing :D // I normally have a helper function like this.
const asyncEvent = (element, eventName, options) => {
let cancelled;
let teardown;
const promise = new Promise((resolve) => {
if (cancelled) return;
const handle = (...args) => resolve(args);
teardown = () => element.removeEventListener(eventName, handle);
element.addEventListener(eventName, handle, { ...options, once: true });
});
promise.cancel = () => {
if (cancelled) return;
if (teardown) teardown();
cancelled = true;
};
return promise;
};
// I use to write something like this in the useEffect
let cancelled;
let cancelablePromises;
(async () => {
try {
if (image.decode) {
await image.decode();
} else if (!image.complete) {
cancelablePromises = [asyncEvent(image, 'load'), asyncEvent(image, 'error')];
const [event] = await Promise.race(cancelablePromises);
if (event.type !== 'load') throw new Error('Failed to load image');
}
if (!cancelled) setLoaded(true);
} catch (e) {
if (!cancelled) setError('Failed to load image');
}
})();
// The teardown
return () => {
cancelled = true;
if (cancelablePromises) cancelablePromises.forEach(({ cancel: fn }) => fn());
};This is how I normally write this. This code isn't tested. |
|
@EloB Interesting! I think that we need to render the image with the src server side. I have tried the following but it doesn't seem better: diff --git a/packages/material-ui/src/Avatar/Avatar.js b/packages/material-ui/src/Avatar/Avatar.js
index ff28b170e..1c7bf0cc6 100644
--- a/packages/material-ui/src/Avatar/Avatar.js
+++ b/packages/material-ui/src/Avatar/Avatar.js
@@ -56,6 +56,16 @@ export const styles = theme => ({
},
});
+let firstRender = true;
+
+function useFirstRender() {
+ React.useEffect(() => {
+ firstRender = false;
+ }, []);
+
+ return firstRender;
+}
+
function useLoaded({ src, srcSet }) {
const [loaded, setLoaded] = React.useState(false);
@@ -71,10 +81,12 @@ function useLoaded({ src, srcSet }) {
image.src = src;
image.srcSet = srcSet;
image.onload = () => {
- if (!active) {
- return;
- }
- setLoaded('loaded');
+ image.decode().then(() => {
+ if (!active) {
+ return;
+ }
+ setLoaded('loaded');
+ })
};
image.onerror = () => {
if (!active) {
@@ -110,8 +122,12 @@ const Avatar = React.forwardRef(function Avatar(props, ref) {
// Use a hook instead of onError on the img element to support server-side rendering.
const loaded = useLoaded({ src, srcSet });
+ const isFirstRender = useFirstRender();
const hasImg = src || srcSet;
- const hasImgNotFailing = hasImg && loaded !== 'error';
+ const hasImgNotFailing = isFirstRender ? hasImg && loaded !== 'error' : loaded === 'loaded';
if (hasImgNotFailing) {
children = ( |
|
@oliviertassinari I think I can live with the initial render being off but after that it somehow makes these beautiful animation you guys done :) For instance that ripple effect on button is amazing :) I've rewrote the code in the previous message to handle error. Using that decode would not block as I understand it correctly. Maybe use that "Fade" component could be handy here. |
Closes #16161
Before
After
