Component should have two "modes", controlled and uncontrolled, just like text inputs

[rpearce]

Issue stems from the discussion and initial issue on #48.

Initial Issue:

Should be checking this.props against nextProps to see if props have
updated. Was instead checking against .state which could be different
even if isZoomed prop never changed.

To reproduce the issue I'm seeing, setting a prop to change after 10 seconds and then zoom into an image. Even though the prop you change isn't isZoomed the image will unzoom.
– @rajit

Diagnosis & Solution Plan

I think that this might be related to this comment of mine from February. I would think that, to follow the React way, props.isZoomed should not be able to be overridden by internal state except by a call to an event handler.

I would say the component should have two “modes”: uncontrolled, in which it handles its own state in state, and controlled, in which a click fires the onUnzoom callback and does nothing else. A controlled component should expect its props to be adjusted for it, in response to an event, just as a controlled <input> doesn’t display a letter until its value prop is changed in response to onChange. This mode change could be based on whether or not a value is provided for isZoomed, controlled if the prop is provided and uncontrolled if it’s undefined.
– @cbothner

Further thoughts

TBD

[rpearce]

@cbothner Could you expand a little more on what you mean by this sentence/phrase?

and controlled, in which a click fires the onUnzoom callback and does nothing else

What I'm understanding (correctly or incorrectly) from your analysis is that if isZoomed is provided as a prop, then the internal state-tracking is disabled and should only zoom/unzoom based on whether the provided props change. Otherwise, it manages its own internal state, and maybe if isZoomed is passed as a prop after it's already been mounted without that prop initially, there's a warning that is displayed, and it ignores it.

[rpearce]

I'm going to get started on this this weekend and not let it linger.

[cbothner]

Sorry for the delay on this. Yes, that is what I mean. That’s exactly what happens with <input /> elements in React, too. If you pass a value prop to a component that has already mounted, you get this error:

A component is changing a controlled input of type %s to be uncontrolled. 
Input elements should not switch from controlled to uncontrolled (or vice versa). 
Decide between using a controlled or uncontrolled input element for the lifetime
of the component. More info: https://fb.me/react-controlled-components

[rpearce]

Getting started on this.

[rpearce]

Edit: This is gonna take a while... lots to change. I'll definitely need some review on this when I get something workable

[cbothner]

maybe if isZoomed is passed as a prop after it's already been mounted without that prop initially, there's a warning that is displayed, and it ignores it.

One other thought: display the warning, definitely, but I think it’s okay to leave the behavior undefined if this does happen. When I have seen that error for inputs in React (due to accidentally passing value={undefined} at mount), the program can often recover and remain functional.

I would imagine it could work out in perhaps the most common case that is likely to come up. Transitioning from isZoomed={undefined} to isZoomed={true}, the naive implementation might actually do the right thing (zoom the component and continue in controlled mode). I guess this is just meant to say that if it seems ignoring it adds a lot of code complexity in the name of handling bad input, it would probably be fine to warn and then leave that logic out.

[rpearce]

@cbothner What do you think of this concept? Because there are a lot of if statements required based on "was isZoomed passed in on instantiation?", I'm considering having different sub-components that just do what they're supposed to instead of all the logic (which makes my head hurt)

[rpearce]

...following the previous comment:

I reckon that the controlled component might not even need to manage any of its own state, since it receives that state via the props, unless there are some weird timing issues that require it

[cbothner]

Doesn’t the animation throw a wrench in “no internal state?” I just spent a few minutes looking through React’s code for input elements to learn from how they do it. When it’s controlled, they force the DOM node value to the prop value with every update, if it’s not already right. When props.value is null, they just fall back to the default browser behavior. But since we don’t have default browser behavior for zooming, we’d have to implement that.

I imagine this:

ImageZoom component has only the internal state isZoomed (boolean) and a child <Zoom isZoomed={this.props.isZoomed != null ? this.props.isZoomed : this.state.isZoomed} ... />. ImageZoom’s handleZoom and handleUnzoom call this.props.onZoom etc and only update state if this.props.isZoomed == null (using == on purpose because it matches null or undefined)

Zoom component is the portal; handles animation and the loading of a high res image but has no internal state about whether or not it should be visible. (Animation in React is not my forte, so I’ll defer to you for the details here)

This way, I don’t think you need any logic for “if isZoomed was passed on mount” or anything. The Zoom component always has a value for its prop isZoomed, so it will always do the right thing. And then for the ImageZoom component, you can throw an error on componentWillReceiveProps if isZoomed was null and is now set, but even still it might just work, since the Zoom component will see a change from state.isZoomed to props.isZoomed no differently.

Does any of that make sense?

[rpearce]

Okay I'll take a few stabs at this over this week and see where we end up. Pretty slammed at present, but will see what I can do.

[rpearce]

Sorry y'all, haven't had any time to devote to this. Sigh. If anybody else wants to take a stab at it, by all means take it over; otherwise, I'm hoping for having time in the next couple of days.

[rpearce]

Good news! I'll be spending a good chunk of a day next week fixing this issue and making a couple of other changes.

[rpearce]

This should now be addressed in version 2.0.0