regression when using animation from 0.4.3

[chrisdrackett]

I was using the following code in 0.4.3 without any issues:

     <Manager>
        <Target component={null}>
          <Button label="button" onClick={this.togglePopover} />
        </Target>
        <VelocityTransitionGroup
          enter={{
            animation: {
              opacity: [1, 'easeOutQuart'],
              scale: [1, [600, 35]],
            },
            duration: 300,
          }}
          leave={{
            animation: {
              opacity: [0, 'easeOutQuart'],
              scale: [0.3, [500, 35]],
            },
            duration: 150,
          }}
          runOnMount
        >
          {this.state.isOpen &&
            <Popper placement="bottom">
              Popper!
            </Popper>}
        </VelocityTransitionGroup>
      </Manager>

I rewrote this code after updating to 0.5.0:

      <Manager>
          <Target>
            {({ targetRef }) => (
              <Button
                label="button"
                ref={targetRef}
                onClick={this.togglePopover}
              />
            )}
          </Target>
          <VelocityTransitionGroup
            enter={{
              animation: {
                opacity: [1, 'easeOutQuart'],
                scale: [1, [600, 35]],
              },
              duration: 300,
            }}
            leave={{
              animation: {
                opacity: [0, 'easeOutQuart'],
                scale: [0.3, [500, 35]],
              },
              duration: 150,
            }}
            runOnMount
          >
            {this.state.isOpen &&
              <Popper placement="bottom">
                {({ popperRef, popperStyle, popperPlacement }) => (
                  <div
                    ref={popperRef}
                    style={popperStyle}
                    data-placement={popperPlacement}
                  >
                    Menu?
                  </div>
                )}
              </Popper>}
          </VelocityTransitionGroup>
      </Manager>

I'm sure there is a possibility I'm doing something wrong and I'll keep hacking on it, but I figured it was worth raising!

[FezVrasta]

I liked more the old component= API 😞 (sorry for the OT)

[souporserious]

Sorry about the API changes, I think it's finally in a stable spot now. Next release should be 1.0.0.

@FezVrasta I knew you weren't going to like the change! haha. I think this way will be better in the long run since it's more explicit with what's going on.

@chrisdrackett what isn't working? If you use just use a Popper normally without a child function does everything work?

I'm guessing that you'll probably need to pass down some info from VelocityTransitionGroup to your element, like the style property. If that doesn't work, I'm happy to see how we can make this work better with things like animation as I'll be wanting this too :)

[souporserious]

I think I know what it is! Right now, no props get passed through when using a child function so they get lost in the Popper component. I'll try and get this fixed ASAP. Thanks for filing an issue!

Thoughts on what it should look like?

Thinking something like:

<Popper>
  {({ popperRef, popperStyle, popperPlacement, props }) => (
    <div
      ref={popperRef}
      className="popper"
      style={popperStyle}
      data-placement={popperPlacement}
      {...props}
    >
      ...
    </div>
  )}
</Popper>

or

<Popper>
  {popperProps => (
    <div
      className="popper"
      {...popperProps}
    >
      ...
    </div>
  )}
</Popper>

[FezVrasta]

Why do we need to pass a function down as children? Wouldn't a component work the same?

[souporserious]

Not always 😕 because we need access to the DOM node, we can't pass a ref down reliably since it could point to a component's class instance if it doesn't return an element. When we use findDOMNode it took care of this, but it has issues. This is the reasoning for a child function, this way you can thread the ref down however you need to.

[chrisdrackett]

honestly I also miss just being able to use component= for this and other issues, but I understand trying to get away from findDOMNode. I'll need to think about how to use this with my mostly functional presentational components. Before they all just worked, but now I'll need to wrap them in something or similar, which will effect my styling.

I think either of the above would work. I personally prefer the second example (with popperProps). That being said, if users want to add their own styles or similar it might cause more issues than the more explicit route.

[souporserious]

I feel you on it, I really liked component as well since it was much more simple. But, I guess simple doesn't always mean other things will be simple 😅

I think I'm going to go with just popperProps. It will merge whatever components props are above it so users should still be able to add styles just fine since you can do something like the following..

<Popper>
  {popperProps => (
    <div
      {...popperProps}
      className="popper"
      style={{
        ...myCustomStyles,
        ...popperProps.style
      }}
    >
      ...
    </div>
  )}
</Popper>

[souporserious]

@FezVrasta can I have a transform above a Popper?

Something like this will not reposition the Popper anymore, it just stays where it originally was.

I'm running into issues trying to get animations working 😩 I think we need to explicitly pass all the props down since in a case like this if an animation was messing with transforms then it needs to live above or below the Popper.

[souporserious]

@chrisdrackett so I finally got animations working with a Popper using React Motion UI Pack. It looks like the following..

<Transition
  component={false}
  enter={{ opacity: 1, scale: 1 }}
  leave={{ opacity: 0, scale: 0.9 }}
>
  {this.state.isOpen &&
    <Popper>
      {({ popperProps, restProps }) => (
        <div {...popperProps}>
          <div {...restProps}> // animations coming through here
            Animated Popper 🎉
          </div>
        </div>
      )}
    </Popper>}
</Transition>

If the animation is using a transform it will need to be applied to a seperate element since it will stomp on the Popper logic.

Does that look good to you?

[chrisdrackett]

yeah, if not a little messy. I'm curious about the use cases where a user would need access to <div {...popperProps}>. My currently gut feeling is to keep that bit internal (yes, you are creating a div, but its being positioned absolutely so it can't affect flow)?

[chrisdrackett]

I guess that in that case you can just use Popper without using the functional child, correct?

[souporserious]

Yes, this is exactly what I do at my work. I use a Popper as the outer element and just let that care about positioning. Then the inner element can be whatever user defined component.

If you're doing something like that, then you shouldn't even need a child function, that's best used for gluing the Popper component to another component. Or in the case of VelocityTransitionGroup and Transition pulling the Popper component apart and placing props in the necessary spot.

However, if you do want to still allow access to the ref and you're using a child function, you could do something like the following...

<Popper>
  {({ popperProps }) =>
    <div
      ref={node => {
        props.innerRef(node) // or whatever to give access to the ref still
        popperProps.ref(node)
      }}
      style={popperProps.style}
      data-placement={popperProps.placement}
    >
      ...
    </div>
  }
</Popper>

It's a little messy, but I'm not sure of any other way without findDOMNode. Definitely open to any ideas you have :)

[chrisdrackett]

I'll play with it more when you push your work. I have a couple ideas, but they are hard to test at the moment :D

[souporserious]

Should be good to go now with the latest release 2109ac0 🎉 feel free to reopen or file any new issues you find 😅 thanks for working through it with me!