[Avatar] Makes Avatar Image load only if Image load without error

[dleroux]

WHY are these changes introduced?

Resolves https://github.com/Shopify/shopify/issues/174824

The default option we want for our Avatar component is to show the logged in user initials if an avatar image is not found. Gravatar gives us the possibility of providing a default image when an image is not found or to return a 404.

In this pr we attempted to return a transparent image. This was done with the following option in the query:

avatar(fallback: TRANSPARENT) {
      src
    }

The idea being that if the fallback was a transparent image it would simply lay over the Initials and wouldn't be visible.

However the Initials are in circle coloured background. As a result when an image was present it would show through pr

WHAT is this pull request doing?

This PR is taking the is relying on gravatar returning a 404.

avatar(fallback: NOT_FOUND) {
      src
    }

If an image requested returns a 404 the onError attribute of the image tag will trigger and no image will be outputted.

In previous attempts server side rendering was causing the onLoad and onError of the image to not trigger. This PR adds the image only on the componentDidMount Lifecycle.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page, Avatar, Image, Stack} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <Page title="Playground">
        <Stack>
          <Avatar size="large" initials="DL" customer />
          <Avatar
            size="large"
            initials="DL"
            source="https://avatars2.githubusercontent.com/u/1229901?s=80&v=4"
          />
          <Avatar size="large" initials="DL" source="https://bla" />
        </Stack>
        <br />
        <Image
          alt="Just an image"
          source="https://avatars2.githubusercontent.com/u/1229901?s=80&v=4"
        />
      </Page>
    );
  }
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated UNRELEASED.md

[dleroux]

Leaving this as a WIP until we can verify that this approach will work in production.

[danrosenthal]

I have some concerns about calling setState in componentDidUpdate. Is there anything we can do here as an alternative? Can it be guarded so it's not called needlessly?

[tmlayton]

A couple questions:

• Is this still a WIP?
• Can you isolate the commit which address why Update Avatar and Image component to handle onError with images. #557 had to be reverted?

[tmlayton]

I think a better approach here is to use !isServer (easy to test) and getDerivedStateFromProps for handling changes to the source prop

[dleroux]

I’m okay either way. I prefer taking advantage of the lifecycle hooks, it feels more like a React approach but I’ll give this a shot.

The other PR was reverted because it broke in prod because of an issue with how Avatar was being exported.

[tmlayton]

I’m okay either way. I prefer taking advantage of the lifecycle hooks, it feels more like a React approach but I’ll give this a shot.

I would disagree – just because we use a lifecycle hook doesn’t necessarily mean it is a better React approach. In this case, it adds complexity (extra state and lifecycle hook) and is harder to test. It also adds an extra render we can avoid:

constructor -> render -> componentDidMount -> setState -> render -> componentDidUpdate

[dleroux]

@tmlayton all changes have been applied and added a test for the isServer. Once you approve we can retry a test on web.

[danrosenthal]

Code changes, test coverage, and the getDerivedStateFromProps approach in particular LGTM. I guess we give this another try in web at some point this week. What do you think @lemonmade and @tmlayton?

Stale review

[dleroux]

@danrosenthal this should ensure we don't get the issue we experienced in web. There's no need to export * now that Avatar is a class component.