joshwcomeau / react-flip-move

Effortless animation between DOM changes (eg. list reordering) using the FLIP technique.
http://joshwcomeau.github.io/react-flip-move/examples
MIT License
4.09k stars 258 forks source link

Add wrapperless mode #201

Closed tobilen closed 6 years ago

tobilen commented 6 years ago

addresses https://github.com/joshwcomeau/react-flip-move/issues/200

With this PR, it will be able to pass false to FlipMoves typeName attribute to have it render out its childs unwrapped (yay react 16). The downside: A dom node has to be passed in as anchor, so it knows where to anchor its animations. If its ommitted, we fall back to findDomNode to get the components parent. you also get lots of warnings free of charge!

I've added tests, a story in the "basic" chapter, commented code where appropriate and modified the readme.

i've also upped some version numbers, namely enzyme + adapter. some of the weird behavior on state updates should be gone now.

MrSauceman commented 6 years ago

Great work here. I really appreciate you tackling this so quickly.

One comment - I'm wondering if we could do without the warning? In the situation of stateless parents, which cannot be given refs, I would want it to choose the parent node by default, and don't need to see a warning. Maybe just a note in the docs about using the anchor prop if you don't want to use the parent.

In what I'm working on, I have a dynamic parent to the FlipMove component that may or may not be stateless. I wouldn't be able to use a ref in this case and would always get this warning.

Hypnosphi commented 6 years ago

@MrSauceman in your case you could introduce a convention that any component, stateless or not, that can be used to wrap its children with a dom node exposes a prop which is a ref to that node, like this:

const MyDiv = ({wrapperRef, children}) => (
  <div ref={wrapperRef}>{children}</div>
)
MrSauceman commented 6 years ago

@Hypnosphi But here you are wrapping your component, exactly what we are trying to avoid with this PR.

Hypnosphi commented 6 years ago

MyDiv is just an example stateless component, not something you should introduce. You can bring your own example of what is used as a parent to FlipMove and I'll show you what can be added there to allow getting the underlying DOM node reference

MrSauceman commented 6 years ago

If for instance the parent component is stateless and from a third party npm package, you do not have the ability to add a ref property.

tobilen commented 6 years ago

I realize having to pass an anchor seems a bit inconvenient, but its required to provide the component with all the information it needs without it "stepping outside of its boundaries". Facebook phrases it like this:

findDOMNode is an escape hatch used to access the underlying DOM node. In most cases, use of this escape hatch is discouraged because it pierces the component abstraction. (https://reactjs.org/docs/react-dom.html#finddomnode)

I tend to agree. A component should not have to look outside of its DOM in order to work properly. The fact that this solution is able to do it doesnt change the fact that its an antipattern. Maybe post a snippet to demonstrate your problem so we can try to come up with a different solution?

MrSauceman commented 6 years ago

Here is a very simple example that illustrates my problem:

import React from 'react';
import { Grid } from 'semantic-ui-react';
import FlipMove from 'react-flip-move';

export default class MyGrid extends React.Component {
  constructor() {
    super();
    this.state = {
      items: [
        "Item 1",
        "Item 2",
        "Item 3",
        "Item 4",
        "Item 5"
      ]
    };
  }

  render() {
    return (
      <Grid ref={(grid) => { this.grid = grid; }}>
        <FlipMove duration={750} easing="ease-out">
          {this.state.items.map(item => (
            <div className="column" key={item}>
              {item}
            </div>
          ))}
        </FlipMove>
      </Grid>
    );
  }
}

Adding that ref to the Grid from semantic will not work, and a warning from React appears about using refs with stateless components. This is a very simple example - in my case, wrapping the Grid or its children in a div to add a ref will not work for me due to it altering the html structure.

Here's a full working example: https://codesandbox.io/s/wko5w5nyn7

You can see the console error there from adding the ref to the Grid. Any idea how I'd work around this without introducing an additional wrapper?

Edit: Perhaps there could be an anchorParent prop that when set to true doesn't show the warning? Just an idea.

Hypnosphi commented 6 years ago

In this particular case, you can do this:

<Grid>
  <FlipMove duration={750} easing="ease-out" typeName={false} anchor={this.grid}>
    {this.state.items.map((item, i) => (
      <div className="column" key={item} ref={i === 0 ? (child => { this.grid = child.parentElement; }) : null}>
        {item}
      </div>
    ))}
  </FlipMove>
</Grid>
Hypnosphi commented 6 years ago

BTW, if you're OK with using findDOMNode you can use a helper from semantic-ui-react: https://react.semantic-ui.com/addons/ref

MrSauceman commented 6 years ago

Thank you for the suggestions and all the work here!

joshwcomeau commented 6 years ago

Woo!! Thanks for your work on this :)

Sorry it took me some time to respond; I've been travelling.

There's a couple things I'd like to tweak (namely, some tab -> spaces conversion, and updating the package version + flow-typed directory). I'll create a new PR and request review.

One question, though: Why is the error message in FlipMove instead of using the error-messages warnOnce convention? Is there a cyclic dependency issue? Could this be handled in prop-converter, with all the other validation stuff?

Hypnosphi commented 6 years ago

some tab -> spaces conversion

Why it's not handled by eslint?

tobilen commented 6 years ago

Why is the error message in FlipMove instead of using the error-messages warnOnce convention?

because i forgot those existed. no reason i can see not to use it

joshwcomeau commented 6 years ago

because i forgot those existed. no reason i can see not to use it

Ok! Cool, will update. Thanks!