Give loadingStatus as an input into the function for container

[eedrah]

This will allow people to render different things depending on if the component is still in the loading stage, the unloading stage, or the loaded stage.

A boolean won't work, as there are 3 states, so I was thinking 1 for when it is loading, 0 for when it has loaded successfully, and -1 for when it is showing the "unloader". This will allow logic to fit into trinaries easy. For example, (children, loadingStatus) => loadingStatus ? children : "Image has loaded and been cached!" will show the loader and unloader as normal, and will show the sentence if successful.

I'll go ahead and do a pull request, but I'm open to more discussion if people want to change the specifics.

[eedrah]

Note that this is backwards compatible - when not using the 2nd parameter in the function signature everything works identical to before.

[mbrevda]

Thank you for your contribution to this project!

It looks like you are trying to pass the loading state to the container. There was a request in the past for a similar feature. Could you elaborate on your use case? I'm wondering if there might be a better way for React Image to communicate it's current state to the container/children.

I'm a bit hesitant to add unnecessary code (for simpler maintenance and bug reduction, etc, also, to keep a single source of truth - see below), but if children being rendered are lacking nesesary information, we can definitely see what the best way to pass that information down is.

Currently, the loading state could be inferred by the component being rendered. I.e. if the loader is being rendered than the image is still trying to be loaded. If the unloader is being rendered, the image obviously failed to load. As a side benefit, if there is a specific behavior you'de like to execute for a given state, that could be added to the specific component (i.e. loader/unloader) and you wouldn't even need an if statement to check the state.

Would colocating the logic in the components work for you're use case?

[eedrah]

Thanks for the reply :)

So the use cases that I'm currently using it for (from my fork) are:

• I want to wrap the final <img> in a container, but I don't want the loader or unloader wrapped.
• In another case, I am using your component to display the loader and unloader, but when the image is loaded, I throw away your <img> and use a different image component with the same src, which is now in the cache thanks to your component. (I'll admit, this last one is more coding out of laziness - instead of writing/finding another component that only does the loading/unloading aspect, I just use yours again. But I've already got it bundled into my project, so maybe I should call that efficiency.) This is the one that I gave as an example above: (children, loadingStatus) => loadingStatus ? children : <ZoomableImage src={src} /> will display the un/loader normally, and use the other component without needing to download the image.

the loading state could be inferred by the component being rendered

This is an interesting thought - how would you do this? Via refs or putting state in the un/loaders? I worry that would add considerable complexity though?

I tried to keep the code change small, so in the pull request #209 I managed to change 3 lines/ints. Would that be a small enough change for a feature?

(I also updated the README for the change)

[mbrevda]

I appreciate the effort to keep the change small! Let's work through this and see if and what is necessary.

---

the loading state could be inferred by the component being rendered

This is an interesting thought - how would you do this?

By checking if the child passed to the container is of an instance of the unloader, loader, or an actual <img>. Hence, it feels like passing a trinary is just passing duplicate data.

As an aside, using a trinary isn't very futureproof as this prevents any future additional states. I'd rather use a string state. Which leads to another question: if we're passing a state, perhaps I should just dump the entire <Img> state to the container so that you can also know WHICH url (out of the img[] list) is being rendered (along with everything else we have in state). The downside here that now the internal state has become part of the public API and changes need to be carefully semver'd.

---

Based on your use cases, as well as those raised by @bildja in #192, here are a few possibilities:

1. Pass the entire raw React Image state to the container
2. Pass in a string state
3. Use separate containers for different states (i.e. a loader container, an unloader container, and a wrapper container). This would make the current state even easier to infer and would eliminate the public API issue, as well the need for an if statement. This would also help in keeping the implementation details of React Image from the container.

Thoughts?

[eedrah]

Hmmm...

1. I agree with you on this - passing the whole state could be a bit iffy for future-proofing...
2. Yeah, I went with the trinary because what I was specifically doing at the time made using integers very easy to throw into if statements. But you raise a good point about string states.
3. This is also an option. Maybe something like if (un)loaderContainer is given or null, then use that, else use the original container prop. I think that would be the smallest possible change to do all combination of behaviors that might be wanted (specifying all three independently, and not forcing the loader to use the container

To be honest, I like 2, just because I see it as super simple, I've already written the code (need to change to strings), and it reminds me of an API of a similar library (which I can't remember at the moment, but it's where I got the idea from), but I understand if you prefer 3, if it's more future-proof for maintenance. So I'm pretty indifferent.

What would you like? And more important, what are you willing to merge? 😄

[eedrah]

On second thought - 3 does seem a lot nicer to use...

[eedrah]

Actually, we don't even need containers for the loader and unloader - only an ability to disable the application of the container for the loader/unloader. If people really want to use the container for the un/loader, then they can just include that in the un/loader prop.

How about two boolean props like useContainerForLoader and useContainerForUnloader, but with better names?

[mbrevda]

Actually, we don't even need containers for the loader and unloader

Having a container for loader/unloader allows for advanced application such as fade in/fade out. I think that having three optional containers, defaulting to container - if provided - is the simplest way forward and shouldn't require a major version bump. Three containers should also cover the other requested use cases.

Instead of booleans to prevent the use of container for loader/unloader, would it be simpler to provide overrides? I.e.

<Img
  src="..."
  container={MySpecialWrappingComponenet}
  loader={Loader}
  loaderContainer={img => (img)} // define a "noop" container, so that `container` isn't used
  unloader={Unloader} // will be wrapped by `container` by default
/>

[eedrah]

How does this look? #211

Docs to follow if you like the look of it. (Is there a way to put a pull request on 'pause'?)

[mbrevda]

#211 seems like the way forward, closing this in favor

[mbrevda]

It is now possible to inject a custom image loader with any desired behavior. You can pass it as a prop to the useImage hook or to the component, see more here.