socialtables / react-image-fallback

stop displaying broken images, have another image to fallback on.
111 stars 18 forks source link

src should be nullable #41

Closed stevemao closed 7 years ago

stevemao commented 7 years ago

How I use this component:

If this.props.maybeNull isn't null but it fails to get the image, it falls back to brokenImage

If this.props.maybeNull is null, it also falls back to brokenImage

src={this.props.maybeNull}
fallbackImage={brokenImage}

Alternative I could do

src={this.props.maybeNull || brokenImage}
fallbackImage={brokenImage}

But this is a lot more verbose and fallbackImage (by name) should take care of both use-cases IMO.

Do you think this is a valid use-case? If not, what do you think should be the right pattern?

conorhastings commented 7 years ago

@stevemao sounds valid, a lot of our work actually does this as well, so i would accept a pr that changed this behavior for sure

conorhastings commented 7 years ago

@stevemao sorry just realized this actually is a pr 🤦‍♀️

conorhastings commented 7 years ago

@stevemao can we change this line https://github.com/stevemao/react-image-fallback/blob/c96ce076c9764dc67b1c66327eb5a6de605afa53/src/index.js#L32

stevemao commented 7 years ago

@conorhastings I'm not sure what you want me to change.

conorhastings commented 7 years ago

@stevemao if there's no image or it's null we shouldn't concat it onto the fallbacks array. we also need to check that fallbacks is an array though I think the logic should look something like this

const fallbacksArray = Array.isArray(fallbacks) ? fallbacks : [fallbacks];
const imagesArray = image ? [image].concat(fallbacksArray) : fallbacksArray;
stevemao commented 7 years ago

Actually fallbacks should be nullable too... So we should check if imagesArray[1] is undefined. I'll do a followup pr once we merge this one.

conorhastings commented 7 years ago

@stevemao cool, not sure I agree fallback should be nullable though? what's the thought process there?

conorhastings commented 7 years ago

published as v6.0.0

stevemao commented 7 years ago

I mean like this

<ReactImageFallback
    src="my-image.png"
    fallbackImage={[nullable, "my-backup.png"]}
/>

If nullable is undefined, it throws a very unfriendly error: ReactImageFallback.render(): A valid React element (or null) must be returned. You may have returned undefined, an array or some other invalid object.

If nullable is null, I don't think most users wanna stop right there but fallback to "my-backup.png"

A workaround would be:

<ReactImageFallback
    src="my-image.png"
    fallbackImage={[nullable || "my-backup.png", "my-backup.png"]}
/>