Controlled vs uncontrolled #52

rpearce · 2017-08-13T01:27:48Z

Fixes #30
Fixes #49

when you pass a non-null value to isZoomed on component instantiation, the component should only zoom/unzoom when that isZoomed prop changes. This means that the consumer is in charge of when it's open or closed. Clicking on the image will not do anything. If it is a controlled component but then you do not pass isZoomed with a non-null value, then an error will be raised telling you that you must control it.
if you do not pass true or false to isZoomed, then the component will control itself. This means that if you initially pass isZoomed={null} and then pass isZoomed={true} (or false), then it will throw an error. If it is an uncontrolled component but then you do pass isZoomed with a non-null value, then an error will be raised telling you that you must not control it.

Please help test the sh*t out of this.

rpearce · 2017-08-13T01:28:20Z

Also cc @ismay and @jeremybini for feedback.

rpearce · 2017-08-13T20:14:10Z

It's worth noting that I tried to refactor this completely as part of this PR, even having the zoom image as a "child" component, but then I recalled why it has to be in a "portal" and create a new react node at the end of the <body>: if <ImageZoom /> lives within a container with overflow: hidden, then the zoomed image will never be able to go outside of this container, and this is one of the core reasons the library was built in the first place.

ismay · 2017-08-14T10:44:43Z

Have to say I don't feel qualified to review this in depth. Haven't dealt a lot with controlled components other than just form elements. But from what I can tell it looks good.

cbothner · 2017-08-14T14:36:06Z

This is looking really good! Thanks for your hard work on this.
Only two things stick out to me as unexpected.

Always fire `onZoom` when clicked

In controlled mode, a click on the unzoomed image should not naively trigger a zoom, so you've implemented that perfectly. But I would expect the click to fire the onZoom callback to give the parent component the option of changing this.props.isZoomed. I think that this change is unnecessary.

(Sidebar: I would say that then perhaps the naming of onZoom isn't quite right with this behavior, but in fact it’s the same pattern as React core. The onChange prop of a controlled <input /> is called when the user attempts to change, even while the controller still has the option not to effect the attempted change.)

Refuse to unzoom if `onUnzoom` doesn’t change `props.isZoomed`

A controlled component only changes if its props change, which means that changes that might be initiated from inside the component (handleScroll etc.) can be cancelled by the parent. A scroll or click event handled by the <Zoom /> component in controlled mode should fire onUnzoom and then wait to have its props changed before unzooming—and if that props change never comes nothing should happen.

Test

I tested with this component. Maybe it will be useful to you. As I understand this library (you’re allowed to disagree—it’s your library) it should be zoomable with the button or by clicking on the image, and it should not permit an unzoom until the countdown is elapsed.

class ControlledImageZoom extends Component {
  constructor(props) {
    super(props)

    this.state = { zoomed: false, countdown: 3, allowUnzoom: true }

    this.handleChangeCountdown = e => {
      this.setState({ countdown: e.target.value })
    }

    this.handleZoom = () => {
      this.setState({ zoomed: true, allowUnzoom: false})
      setTimeout(
        () => this.setState({allowUnzoom: true}),
        this.state.countdown * 1000
      )
    }
    this.handleUnzoom = () => {
      if (this.state.allowUnzoom) this.setState({ zoomed: false })
    }
  }

  render() {
    return (
      <div>
        <label>
          Prevent unzoom for
          <input
            type="number"
            value={this.state.countdown}
            onChange={this.handleChangeCountdown}
          />
          seconds
        </label>
        <div>
          <button onClick={this.handleZoom}>Zoom in</button>
        </div>
        <div>
          State:
          <strong>
            {`zoomed: ${this.state.zoomed},
              allowUnzoom: ${this.state.allowUnzoom}`}
          </strong>
        </div>
        <ImageZoom
          isZoomed={this.state.zoomed}
          onZoom={this.handleZoom}
          onUnzoom={this.handleUnzoom}
          image={{
            src: 'nz.jpg',
            alt: 'Picture of Mt. Cook in New Zealand',
            className: 'img',
            style: {
              width: '20em'
            }
          }}
          zoomImage={{
            src: 'nz-big.jpg',
            alt: 'Picture of Mt. Cook in New Zealand',
            className: 'img--zoomed'
          }}
        />
      </div>
    )
  }
}

rpearce · 2017-08-14T20:10:14Z

@cbothner great review. Really.

I agree that onZoom might make more sense as onChange; however, that conceptually combines onZoom and onUnzoom into the same category, as it's something that is changed within the component, as well. If we changed onZoom and onUnzoom to both be onChange, what would the argument be to that callback function be? I think we'd want to specify what the event was (is the thing activated or not?). My first instinct says to return a boolean for if it's activated or not, then I thought an object of what the action/event is made more sense: { event: 'zoom' } / { event: 'unzoom' }. And an object might be wiser long-term in case anything else needs to get shoved in there.

Do you think anything at all should get passed back? How does this sound?

rpearce · 2017-08-14T20:12:39Z

Or should it send back both the original click event and that object or both combined into one?

rpearce · 2017-08-14T20:17:54Z

@cbothner You bring up a good point with those scroll/touch listeners. Should they even be set if it's a controlled component? Following my onZoom / onUnzoom thoughts above, shooting off onChange events when it's not changing from unzoomed to zoomed or vice versa doesn't make sense any more...

cbothner · 2017-08-14T20:30:57Z

I actually don’t think we should combine onZoom and onUnzoom, since those are sensible events in controlled or uncontrolled state. I was really just pointing out that it’s okay for them to mean, semantically, the user is attempting to perform a zoom, in the same way that onChange from a controlled input means the user is attempting to perform a change action.

And certainly we do need to set the listeners even if it is a controlled component. Scrolling or clicking or whatever on the zoomed image still needs to be a way to unzoom an image (by firing the onUnzoom callback. It’s just a question of what component is managing the state and my comments were just intended to make sure that DOM state and React state (even if it’s inherited from a parent component) can’t get out of sync.

rpearce · 2017-08-14T20:37:39Z

Okay thanks for clarifying that. I've been playing with your test component and am close to some more stuff.

rpearce · 2017-08-14T21:31:23Z

I need to take a break. I cannot currently find a way to have this work in a props-passing way, as the Zoom component creates a new React node and is not a child of ImageZoom. The problem currently is that once the Zoom component is created, it controls itself (this includes closing animation) and takes no further input from the ImageZoom "parent". Thus, when it's a controlled component and told to no longer be isZoomed, it just removes the DOM node off the page without any animation. If I really wanted to find a way to contact the Zoom component and tell it to shut itself down, then I could use a custom JavaScript event, but that seems so damn hacky.

This struggle 🚌 makes me feel like I need to rethink some things. TBD.

rpearce · 2017-08-14T21:36:25Z

(rebased with master and put current work up here)

rpearce · 2017-08-20T23:00:23Z

@cbothner you may have issue with what I'm about to describe, so please hear me out:

The more I tried to figure something out to address your thoughts above with regards to the component "reporting back" when it should be closing but without doing the actual closing, the less it made sense to me to have the component care about that and be responsible for such actions. If the consumer has chosen to be in control of this thing, then shouldn't it be the one in control?

Instead, I dug deeper and identified two different sets of controls that determine what the zoom component should respond to:

resize event (always)
everything else (scroll, touch, click on document) (only when it is uncontrolled)

I came to this conclusion when I realized that all the conditional logic had to do with events and what should happen when the consumer is in control and what happens when the component controls itself. Here's what it essentially looks like with this system:

controlled component

<ResizeWrapper>
  <Zoom ... />
</ResizeWrapper>

uncontrolled component

<FullWrapper>
  <ResizeWrapper>
    <Zoom ... />  
  </ResizeWrapper>
</FullWrapper>

but this latter one includes the ResizeWrapper within it, as well.

Since the only communication we have outside of emitting global events is accessing a top-level component through the "portal" and calling some method on that, I have each one of these wrappers clone their child and add a ref to it in order to call unzoom on the Zoom component, which handles its own sort of shutdown sequence and then calls back to the original component.

Here's what I tested with in the example file (first image responds to j (open) and k (close)):

import React, { Component } from 'react'
import ReactDOM from 'react-dom'
import ImageZoom from '../../lib/react-medium-image-zoom'

class App extends Component {
  constructor(props) {
    super(props)
    this.state = { isZoomed: false }
  }
  componentDidMount() {
    document.addEventListener('keyup', (e) => {
      switch (e.keyCode) {
        case 74:
          return this.setState({ isZoomed: true })
        case 75:
          return this.setState({ isZoomed: false })
        default:
          return
      }
    })
  }
  render() {
    return (
      <div className="container">
        <h1>Image Zoom</h1>
        <p>Trust fund seitan chia, wolf lomo letterpress Bushwick before they sold out. Carles kogi fixie, squid twee Tonx readymade cred typewriter scenester locavore kale chips vegan organic. Meggings pug wolf Shoreditch typewriter skateboard. McSweeney's iPhone chillwave, food truck direct trade disrupt flannel irony tousled Cosby sweater single-origin coffee. Organic disrupt bicycle rights, tattooed messenger bag flannel craft beer fashion axe bitters. Readymade sartorial craft beer, quinoa sustainable butcher Marfa Echo Park Terry Richardson gluten-free flannel retro cred mlkshk banjo. Salvia 90's art party Blue Bottle, PBR&amp;B cardigan 8-bit.</p>
        <p>Meggings irony fashion axe, tattooed master cleanse Blue Bottle stumptown bitters authentic flannel freegan paleo letterpress ugh sriracha. Wolf PBR&amp;B art party aesthetic meh cliche. Sartorial before they sold out deep v, aesthetic PBR&amp;B craft beer post-ironic synth keytar pork belly skateboard pour-over. Tonx cray pug Etsy, gastropub ennui wolf ethnic Odd Future viral master cleanse skateboard banjo. Pitchfork scenester cornhole, whatever try-hard ethnic banjo +1 gastropub American Apparel vinyl skateboard Shoreditch seitan. Blue Bottle keffiyeh Austin Helvetica art party. Portland ethnic fixie, beard retro direct trade ugh scenester Tumblr readymade authentic plaid pickled hashtag biodiesel.</p>
        <div>
          <ImageZoom
            image={{
              src: 'bridge.jpg',
              alt: 'Golden Gate Bridge',
              className: 'img'
            }}
            zoomImage={{
              src: 'bridge-big.jpg',
              alt: 'Golden Gate Bridge',
              className: 'img--zoomed'
            }}
            isZoomed={this.state.isZoomed}
          />
        </div>
        <p>Thundercats freegan Truffaut, four loko twee Austin scenester lo-fi seitan High Life paleo quinoa cray. Schlitz butcher ethical Tumblr, pop-up DIY keytar ethnic iPhone PBR sriracha. Tonx direct trade bicycle rights gluten-free flexitarian asymmetrical. Whatever drinking vinegar PBR XOXO Bushwick gentrify. Cliche semiotics banjo retro squid Wes Anderson. Fashion axe dreamcatcher you probably haven't heard of them bicycle rights. Tote bag organic four loko ethical selfies gastropub, PBR fingerstache tattooed bicycle rights.</p>
        <div style={{ float: 'left', margin: '0 1.5em 0 0' }}>
          <ImageZoom
            image={{
              src: 'nz.jpg',
              alt: 'Picture of Mt. Cook in New Zealand',
              className: 'img',
              style: {
                width: '20em'
              }
            }}
            zoomImage={{
              src: 'nz-big.jpg',
              alt: 'Picture of Mt. Cook in New Zealand',
              className: 'img--zoomed'
            }}
          />
        </div>
        <p>Ugh Portland Austin, distillery tattooed typewriter polaroid pug Banksy Neutra keffiyeh. Shoreditch mixtape wolf PBR&amp;B, tote bag dreamcatcher literally bespoke Odd Future selfies 90's master cleanse vegan. Flannel tofu deep v next level pickled, authentic Etsy Shoreditch literally swag photo booth iPhone pug semiotics banjo. Bicycle rights butcher Blue Bottle, actually DIY semiotics Banksy banjo mixtape Austin pork belly post-ironic. American Apparel gastropub hashtag, McSweeney's master cleanse occupy High Life bitters wayfarers next level bicycle rights. Wolf chia Terry Richardson, pop-up plaid kitsch ugh. Butcher +1 Carles, swag selfies Blue Bottle viral.</p>
        <p>Keffiyeh food truck organic letterpress leggings iPhone four loko hella pour-over occupy, Wes Anderson cray post-ironic. Neutra retro fixie gastropub +1, High Life semiotics. Vinyl distillery Etsy freegan flexitarian cliche jean shorts, Schlitz wayfarers skateboard tousled irony locavore XOXO meh. Ethnic Wes Anderson McSweeney's messenger bag, mixtape XOXO slow-carb cornhole aesthetic Marfa banjo Thundercats bitters. Raw denim banjo typewriter cray Tumblr, High Life single-origin coffee. 90's Tumblr cred, Terry Richardson occupy raw denim tofu fashion axe photo booth banh mi. Trust fund locavore Helvetica, fashion axe selvage authentic Shoreditch swag selfies stumptown +1.</p>
        <div>
          <ImageZoom
            image={{
              src: 'gazelle.jpg',
              alt: 'Gazelle Stomping',
              title: "Don't exceed original image dimensions...",
              className: 'img',
              style: {
                width: '80%'
              }
            }}
            shouldRespectMaxDimension={ true }
          />
        </div>
        <p>Scenester chambray slow-carb, trust fund biodiesel ugh bicycle rights cornhole. Gentrify messenger bag Truffaut tousled roof party pork belly leggings, photo booth jean shorts. Austin readymade PBR plaid chambray. Squid Echo Park pour-over, wayfarers forage whatever locavore typewriter artisan deep v four loko. Locavore occupy Neutra Pitchfork McSweeney's, wayfarers fingerstache. Actually asymmetrical drinking vinegar yr brunch biodiesel. Before they sold out sustainable readymade craft beer Portland gastropub squid Austin, roof party Thundercats chambray narwhal Bushwick pug.</p>
        <p>Literally typewriter chillwave, bicycle rights Carles flannel wayfarers. Biodiesel farm-to-table actually, locavore keffiyeh hella shabby chic pour-over try-hard Bushwick. Sriracha American Apparel Brooklyn, synth cray stumptown blog Bushwick +1 VHS hashtag. Wolf umami Carles Marfa, 90's food truck Cosby sweater. Fanny pack try-hard keytar pop-up readymade, master cleanse four loko trust fund polaroid salvia. Photo booth kitsch forage chambray, Carles scenester slow-carb lomo cardigan dreamcatcher. Swag asymmetrical leggings, biodiesel Tonx shabby chic ethnic master cleanse freegan.</p>
        <p>Raw denim Banksy shabby chic, 8-bit salvia narwhal fashion axe. Ethical Williamsburg four loko, chia kale chips distillery Shoreditch messenger bag swag iPhone Pitchfork. Viral PBR&amp;B single-origin coffee quinoa readymade, ethical chillwave drinking vinegar gluten-free Wes Anderson kitsch Tumblr synth actually bitters. Butcher McSweeney's forage mlkshk kogi fingerstache. Selvage scenester butcher Shoreditch, Carles beard plaid disrupt DIY. Pug readymade selvage retro, Austin salvia vinyl master cleanse flexitarian deep v bicycle rights plaid Terry Richardson mlkshk pour-over. Trust fund try-hard banh mi Brooklyn, 90's Etsy kogi YOLO salvia.</p>
      </div>
    )
  }
}

const container = document.querySelector('[data-app]')
ReactDOM.render(<App />, container)

This is what makes the most sense to me, and I'm not quite technically sure how to do it any other way at the moment.

If you can spare the time, what are your thoughts on this? Thank you for helping me.

cbothner · 2017-08-21T14:58:29Z

Hey @rpearce. Thanks so much for the hard work. I'm slammed today, so I'll take a look tomorrow morning.

cbothner · 2017-08-22T14:55:56Z

Hey. Sorry I'm not better able to help implement this; I feel like I'm asking so much work of you while contributing so little.

I see where you're going with this. Certainly resize is an event which the component should always handle—it has nothing to do with zooming or unzooming, so the controlling consumer doesn't need to worry about it. But while I acknowledge that you have no responsibility to cater to my use case, leaving off the other listeners entirely would break this component for me. Without the component calling back, there would be no way to update its props to unzoom but for the consumer to add its own event listeners. That feels wrong to me: the consumer of an input component, controlled or uncontrolled, doesn't need to add keypress listeners to update the text themselves. (So besides, that would heavily increase your documentation burden: how many people would forget, or would forget to add for example the touch listener, and would blame their problems on your package?)

To be honest, I'm forming my expectations for how your component should work from palantir/blueprint's Overlay component (typescript, sorry...). [docs] They don't supply an uncontrolled mode, so their component will never open or close itself without a props change; nevertheless, they've got several ways that the component signals it should be closed by its parent (click on backdrop; click on x button, esc key). The only thing that triggers the rendering of an open or close animation is a props change.

Ignoring their inline mode, it seems they never unmount their portal; rather they simply render children or null based on the isOpen prop. And then when their component needs to signal it should close, e.g. when the user clicks on the backdrop or presses esc, it simply calls onClose. I'm not sure if this is any help. I'm not sure how your cool animation to and from the normal image node works; perhaps it would be incompatible with something like this. But perhaps you can find some inspiration there.

On another smaller note about your last commit here: I think that string refs are deprecated, so you might want to consider avoiding them...

Anyway, seriously thank you for your commitment to making this library better for all of us. I know how hard it is to find the time around everything else we all have to do, and how much pressure we users can put on you maintainers. I've figured out what I need as workarounds, so please don't take any of my comments as an expectation or pressure. Whatever you can do: it's appreciated.

rpearce · 2017-08-22T19:15:14Z

@cbothner Not a problem. That's the burden of maintainership!

I'll have a look around those links, think some more, push stuff around some more and reach out to some colleagues of mine for ideas.

rpearce · 2017-08-22T20:29:35Z

@cbothner after speaking with a colleague and hearing a great "that's why people use libraries" analysis, I'll see about combining the functionalities back together and triggering some sort of callback instead of updating internal state. However, I can't say what that API will look like just yet.

rpearce · 2017-09-06T13:55:06Z

Update on this: I'm still traveling in the USA but will be back in New Zealand starting next week where I'll pick this back up.

…sumer when it would normally close

rpearce · 2017-09-20T07:24:12Z

@cbothner Hello again! I've been granted a lot of time this week to work on this. If you get some time, can you check this out again and see if it is more to your liking? I need to do some more edge case testing, and there's some loading animation stuff that I'd still like to work on (if you load it as controlled and isZoomed is true, the initial set of animations are a little wonky) (done). Apart from that, here's what I've done:

moved components, helpers and defaults to their own files for my sanity (finally)
set it up so that if it's a controlled component, the onUnzoom callback will be called, notifying you that we would normally shut it down
this should work in both the controlled/uncontrolled cases

Other things:

investigated ad nauseum if I could do the zoom component as a child component and not a totally different React render instance, but we can't because of the issues position: fixed elements have inside of any parent that has transform set to a value not equal to none. Basically, the CSS spec doesn't allow for it as it creates a new stacking context and set of coordinates that the fixed element now adheres to instead of the viewport. At least I tried!
I've added a few issues, and if you have any opinion on either those or have any of your own, please add them as I'm focused on this for a couple of days

rpearce · 2017-09-20T22:10:15Z

I feel pretty good about this after testing in another app. If there are any adjustments to be made that y'all can think of, we can cut a patch release. cc @rajit

cbothner · 2017-09-21T03:41:22Z

Looking forward to putting eyes on this (and those new issues) tomorrow—today was crazy and I apologize for not getting around to it. From what you’ve described I’m thrilled. One quick thought is that you should not feel bad about having to do a new render instance. In my experience that is actually the right way to do this kind of thing; portals are a yet-undocumented upcoming feature of React 16.

Of course the new implementation is declarative and wickedly optimized... shiny new toys soon!

rpearce · 2017-09-21T03:55:56Z

@cbothner thanks for those links. Also keep a heads up -> I've got 2 bug fixes coming in hot, as well as implementing a basic React Storybook to show all the different options.

ismay · 2017-09-21T12:39:48Z

Works really well for me 👍

rpearce force-pushed the controlled-uncontrolled branch 3 times, most recently from cf7bd6b to 6c55676 Compare August 13, 2017 20:09

rpearce changed the title ~~Controlled vs uncontrolled~~ WIP :: Controlled vs uncontrolled Aug 14, 2017

rpearce added feature bug labels Aug 14, 2017

rpearce force-pushed the controlled-uncontrolled branch 2 times, most recently from 30ae8c1 to 864073a Compare August 14, 2017 21:35

rpearce force-pushed the controlled-uncontrolled branch 5 times, most recently from 36bae68 to 58f3566 Compare August 20, 2017 22:52

Controlled vs uncontrolled

5bedd22

rpearce force-pushed the controlled-uncontrolled branch from 58f3566 to 5bedd22 Compare August 20, 2017 22:58

rpearce added 2 commits September 19, 2017 16:28

Move components, defaults and helpers to their own files

e6f9fa2

Combined event wrappers; got controlled component calling back to con…

1e3e9f3

…sumer when it would normally close

rpearce force-pushed the controlled-uncontrolled branch from 50bcdac to 1e3e9f3 Compare September 20, 2017 07:17

rpearce added 2 commits September 21, 2017 09:01

add setTimeout of 0 if isZoomed is true on mount

2210fdc

Updated dev tools and added package-lock

8edfe63

rpearce changed the title ~~WIP :: Controlled vs uncontrolled~~ Controlled vs uncontrolled Sep 20, 2017

rpearce added 2 commits September 21, 2017 10:05

added changelog and entry for 2.0.0

feaa034

2.0.0

9738884

rpearce merged commit 98c318c into master Sep 20, 2017

rpearce deleted the controlled-uncontrolled branch September 20, 2017 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controlled vs uncontrolled #52

Controlled vs uncontrolled #52

rpearce commented Aug 13, 2017 •

edited

Loading

rpearce commented Aug 13, 2017

rpearce commented Aug 13, 2017

ismay commented Aug 14, 2017

cbothner commented Aug 14, 2017

rpearce commented Aug 14, 2017

rpearce commented Aug 14, 2017 •

edited

Loading

rpearce commented Aug 14, 2017

cbothner commented Aug 14, 2017

rpearce commented Aug 14, 2017

rpearce commented Aug 14, 2017

rpearce commented Aug 14, 2017

rpearce commented Aug 20, 2017

cbothner commented Aug 21, 2017

cbothner commented Aug 22, 2017

rpearce commented Aug 22, 2017

rpearce commented Aug 22, 2017 •

edited

Loading

rpearce commented Sep 6, 2017

rpearce commented Sep 20, 2017 •

edited

Loading

rpearce commented Sep 20, 2017

cbothner commented Sep 21, 2017 •

edited

Loading

rpearce commented Sep 21, 2017

ismay commented Sep 21, 2017

Controlled vs uncontrolled #52

Controlled vs uncontrolled #52

Conversation

rpearce commented Aug 13, 2017 • edited Loading

rpearce commented Aug 13, 2017

rpearce commented Aug 13, 2017

ismay commented Aug 14, 2017

cbothner commented Aug 14, 2017

Always fire onZoom when clicked

Refuse to unzoom if onUnzoom doesn’t change props.isZoomed

Test

rpearce commented Aug 14, 2017

rpearce commented Aug 14, 2017 • edited Loading

rpearce commented Aug 14, 2017

cbothner commented Aug 14, 2017

rpearce commented Aug 14, 2017

rpearce commented Aug 14, 2017

rpearce commented Aug 14, 2017

rpearce commented Aug 20, 2017

cbothner commented Aug 21, 2017

cbothner commented Aug 22, 2017

rpearce commented Aug 22, 2017

rpearce commented Aug 22, 2017 • edited Loading

rpearce commented Sep 6, 2017

rpearce commented Sep 20, 2017 • edited Loading

rpearce commented Sep 20, 2017

cbothner commented Sep 21, 2017 • edited Loading

rpearce commented Sep 21, 2017

ismay commented Sep 21, 2017

rpearce commented Aug 13, 2017 •

edited

Loading

Always fire `onZoom` when clicked

Refuse to unzoom if `onUnzoom` doesn’t change `props.isZoomed`

rpearce commented Aug 14, 2017 •

edited

Loading

rpearce commented Aug 22, 2017 •

edited

Loading

rpearce commented Sep 20, 2017 •

edited

Loading

cbothner commented Sep 21, 2017 •

edited

Loading