Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweaks to new Wrapperless mode #204

Merged
merged 6 commits into from
Nov 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
{
"javascript.validate.enable": false,
"eslint.enable": true
}
}
111 changes: 73 additions & 38 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,6 @@ Flip Move uses the [_FLIP technique_](https://aerotwist.com/blog/flip-your-anima
* <a href="http://joshwcomeau.github.io/react-flip-move/examples/#/scrabble" target="_blank">__Scrabble__</a>
* <a href="http://joshwcomeau.github.io/react-flip-move/examples/#/laboratory" target="_blank">__Laboratory__</a>

## Table of Contents

* [Installation](#installation)
* [Features](#features)
* [Quickstart](#quickstart)
* [Compatibility](#compatibility)
* [Enter/Leave Animations](https://github.com/joshwcomeau/react-flip-move/blob/master/documentation/enter_leave_animations.md)
* [API Reference](https://github.com/joshwcomeau/react-flip-move/blob/master/documentation/api_reference.md)
* [Gotchas](#gotchas)
* [Known Issues](#known-issues)
* [Contributions](#contributions)
* [Development](#development)
* [Flow support](#flow-support)
* [License](#license)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TOC was quite outdated, and I realized it'll always be problematic. This doc isn't so long now that API Reference is its own thing, so I ditched the TOC.




## Installation
Expand Down Expand Up @@ -114,45 +99,95 @@ Curious how this works, under the hood? [__Read the Medium post__](https://mediu

---

### HTML Attributes
### Wrapping Element

FlipMove creates its own DOM node to wrap the children it needs to animate. Sometimes, you'll want to be able to pass specific HTML attributes to this node.
By default, FlipMove wraps the children you pass it in a `<div>`:

All props other than the ones listed above will be delegated to this new node, so you can apply them directly to FlipMove. For example:
```jsx
// JSX
<FlipMove>
<div key="a">Hello</div>
<div key="b">World</div>
</FlipMove>

```html
// HTML
<div>
<FlipMove typeName="ul" className="row" style={{ backgroundColor: 'red' }}>
<li className="col">Column 1</li>
<li className="col">Column 2</li>
</FlipMove>
<div>Hello</div>
<div>World</div>
</div>
```

FlipMove passes the `className` and `style` props along to the `ul` that needs to be created. Here's how it renders:
Any unrecognized props to `<FlipMove>` will be delegated to this wrapper element:

```html
<div>
<ul class="row" style="background-color: red">
<li class="col">Column 1</li>
<li class="col">Column 2</li>
</ul>
```jsx
// JSX
<FlipMove className="flip-wrapper" style={{ color: 'red' }}>
<div key="a">Hello</div>
<div key="b">World</div>
</FlipMove>

// HTML
<div class="flip-wrapper" style="color: red;">
<div key="a">Hello</div>
<div key="b">World</div>
</div>
```

This works for all HTML props - there's no validation.
You can supply a different element type with the `typeName` prop:

You can pass `false` to `typeName` and a valid HTMLElement reference as `anchor` to render the children without a wrapping DOM node. Note that you will have to use react 16.0.0 or higher, and that all other props (such as `className` or `style`) will have no effect.
```html
<div ref={node => this.myCustomParent = node}>
<FlipMove typeName={false} anchor={this.myCustomParent}>
<li className="col">Column 1</li>
<li className="col">Column 2</li>
```jsx
// JSX
<FlipMove typeName="ul">
<li key="a">Hello</li>
<li key="b">World</li>
</FlipMove>

// HTML
<ul style="color: red;">
<li key="a">Hello</li>
<li key="b">World</li>
</ul>
```

Finally, if you're using React 16 or higher, and Flip Move 2.10 or higher, you can use the new "wrapperless" mode. This takes advantage of a React Fiber feature, which allows us to omit this wrapping element:

```jsx
// JSX
<div className="your-own-element">
<FlipMove typeName={null}>
<div key="a">Hello</div>
<div key="b">World</div>
</FlipMove>
</div>

// HTML
<div class="your-own-element">
<div key="a">Hello</div>
<div key="b">World</div>
</div>
```

If no anchor is provided, react-flip-move will try to use the components parent node. This, however, is a costly operation that violates component separation principles and thus is [highly discouraged](https://reactjs.org/docs/react-dom.html#finddomnode).
Wrapperless mode is nice, because it makes FlipMove more "invisible", and makes it easier to integrate with parent-child CSS properties like flexbox. However, there are some things to note:

- This is a new feature in FlipMove, and isn't as battle-tested as the traditional method. Please test thoroughly before using in production, and report any bugs!
- FlipMove does some positioning magic for enter/exit animations - specifically, it temporarily applies `position: absolute` to its children. For this to work correctly, you'll need to make sure that `<FlipMove>` is within a container that has a non-static position (eg. `position: relative`), and no padding:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I noticed in Storybook; It's kind of a bummer :( and makes me think that wrapperless mode will likely cause more trouble than it's worth... but I figure we'll see. If we get a ton of confusing issues, maybe we can add a check that console.warns when the parent DOM element isn't configured correctly.

Copy link
Collaborator

@tobilen tobilen Nov 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we already have the dom node, those styles could be overwritten after findDOMNode:

  findDOMContainer = () => {
    // eslint-disable-next-line react/no-find-dom-node
    const domNode = ReactDOM.findDOMNode(this);
    const parentNode = domNode && domNode.parentNode;

    // This ought to be impossible, but handling it for Flow's sake.
    if (!parentNode || !(parentNode instanceof HTMLElement)) {
      return;
    }

    // Make sure parentNode has position relative and no padding
    parentNode.style.position = 'relative';
    parentNode.style.padding = 0;

    this.parentData.domNode = parentNode;
  };

should be mentioned in the docs though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parentNode.style.position = 'relative';

We can do that only if window.getComputedStyle(parentNode) === 'static')
And we can't change the padding as it would break the layout

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so I thought about applying a "default" relative position if it isn't set to something other than static, but it feels a bit like it crosses a line; we'd be mutating nodes outside of Flip Move's control. It could cause confusion for users if they have absolutely-positioned things down-tree.

So either we apply position: relative, which fixes FlipMove but breaks their layout, or we do nothing, and it breaks enter/exit animations. The latter is clearly a FlipMove issue, and so they'll come here and we can advise them... but the former might appear to them as a CSS issue / they won't understand why things are broken, why a random position: relative is being applied to their div...

Maybe we can apply relative positioning if it's static, and also console.warn to let them know what's happening? But, if the user doesn't need enter/leave animations, they don't need this position: relative, and so the warning would just be console spam.

That said, it's really rare that position: static is actually desirable (I've seen a trend where people just add a * { position: relative } global CSS rule), so I think it's probably best to optimize for the common usecase.

Gonna do it in a subsequent PR to make reviews easier, but I'll apply @Hypnosphi's static check, and issue a console.warn when the change is necessary.


```jsx
// BAD - this will cause children to jump to a new position before exiting:
<div style={{ padding: 20 }}>
<FlipMove typeName={null}>
<div key="a">Hello world</div>
</FlipMove>
</div>

// GOOD - a non-static position and a tight-fitting wrapper means children will
// stay in place while exiting:
<div style={{ position: 'relative' }}>
<FlipMove typeName={null}>
<div key="a">Hello world</div>
</FlipMove>
</div>
```

---

Expand Down
8 changes: 5 additions & 3 deletions documentation/api_reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,15 +277,17 @@ In general, it is advisable to ignore the `domNodes` argument and work with the

### `typeName`

| **Accepted Types:** | **Default Value** |
|---------------------|-------------------|
| `String` | 'div' |
| **Accepted Types:** | **Default Value** |
|----------------------|-------------------|
| `String` | `null` | 'div' |
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this in #202, and didn't see any strong opinions; my preference is that we use null over false, since it feels more "pure" semantically.

If y'all think there's value in also supporting false (or if there's an argument I haven't thought of for why false makes more sense?), happy to update :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer null



Flip Move wraps your children in a container element. By default, this element is a `div`, but you may wish to provide a custom HTML element (for example, if your children are list items, you may wish to set this to `ul`).

Any valid HTML element type is accepted, but peculiar things may happen if you use an unconventional element.

With React 16, Flip Move can opt not to use a container element: set `typeName` to `null` to use this new "wrapperless" behaviour. [Read more](https://github.com/joshwcomeau/react-flip-move/blob/master/README.md#wrapping-elements).

---

### `disableAllAnimations`
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
declare module "react-flip-move" {
declare module 'react-flip-move' {
declare export type Styles = {
[key: string]: string
[key: string]: string,
};

declare type ReactStyles = {
[key: string]: string | number
[key: string]: string | number,
};

declare export type Animation = {
from: Styles,
to: Styles
to: Styles,
};

declare export type Presets = {
elevator: Animation,
fade: Animation,
accordionVertical: Animation,
accordionHorizontal: Animation,
none: null
none: null,
};

declare export type AnimationProp = $Keys<Presets> | boolean | Animation;
Expand All @@ -28,27 +28,27 @@ declare module "react-flip-move" {
bottom: number,
left: number,
height: number,
width: number
width: number,
};

// can't use $Shape<React$Element<*>> here, because we use it in intersection
declare export type ElementShape = {
+type: $PropertyType<React$Element<*>, "type">,
+props: $PropertyType<React$Element<*>, "props">,
+key: $PropertyType<React$Element<*>, "key">,
+ref: $PropertyType<React$Element<*>, "ref">
+type: $PropertyType<React$Element<*>, 'type'>,
+props: $PropertyType<React$Element<*>, 'props'>,
+key: $PropertyType<React$Element<*>, 'key'>,
+ref: $PropertyType<React$Element<*>, 'ref'>,
};

declare type ChildHook = (element: ElementShape, node: ?HTMLElement) => mixed;

declare export type ChildrenHook = (
elements: Array<ElementShape>,
nodes: Array<?HTMLElement>
nodes: Array<?HTMLElement>,
) => mixed;

declare export type GetPosition = (node: HTMLElement) => ClientRect;

declare export type VerticalAlignment = "top" | "bottom";
declare export type VerticalAlignment = 'top' | 'bottom';

declare export type Child = void | null | boolean | React$Element<*>;

Expand All @@ -61,7 +61,7 @@ declare module "react-flip-move" {
disableAllAnimations: boolean,
getPosition: GetPosition,
maintainContainerHeight: boolean,
verticalAlignment: VerticalAlignment
verticalAlignment: VerticalAlignment,
};

declare type PolymorphicProps = {
Expand All @@ -70,37 +70,36 @@ declare module "react-flip-move" {
staggerDurationBy: string | number,
staggerDelayBy: string | number,
enterAnimation: AnimationProp,
leaveAnimation: AnimationProp
leaveAnimation: AnimationProp,
};

declare type Hooks = {
onStart?: ChildHook,
onFinish?: ChildHook,
onStartAll?: ChildrenHook,
onFinishAll?: ChildrenHook
onFinishAll?: ChildrenHook,
};

declare export type DelegatedProps = {
style?: ReactStyles,
anchor?: HTMLElement
};

declare export type FlipMoveDefaultProps = BaseProps & PolymorphicProps;

declare export type CommonProps = BaseProps &
Hooks & {
children?: ChildrenArray<Child>
children?: ChildrenArray<Child>,
};

declare export type FlipMoveProps = FlipMoveDefaultProps &
CommonProps &
DelegatedProps & {
appearAnimation?: AnimationProp,
disableAnimations?: boolean // deprecated, use disableAllAnimations instead
disableAnimations?: boolean, // deprecated, use disableAllAnimations instead
};

declare class FlipMove extends React$Component<FlipMoveProps> {
static defaultProps: FlipMoveDefaultProps
static defaultProps: FlipMoveDefaultProps;
}

declare export default typeof FlipMove
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-flip-move",
"version": "2.9.17",
"version": "2.10.0",
"description": "Effortless animation between DOM changes (eg. list reordering) using the FLIP technique.",
"main": "lib/index.js",
"typings": "typings/react-flip-move.d.ts",
Expand Down Expand Up @@ -95,7 +95,7 @@
"mocha": "3.5.3",
"preact": "8.1.0",
"preact-compat": "3.16.0",
"prettier": "^1.6.1",
"prettier": "1.8.2",
"prop-types": "15.5.8",
"react": "16.0.0",
"react-dom": "16.0.0",
Expand Down
49 changes: 19 additions & 30 deletions src/FlipMove.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,14 @@ class FlipMove extends Component<ConvertedProps, FlipMoveState> {
remainingAnimations = 0;
childrenToAnimate: Array<Key> = [];

// Make sure we dont spam warnings in the console
wasWarned = false;

componentDidMount() {
// Because React 16 no longer requires wrapping elements, Flip Move can opt
// to not wrap the children in an element. In that case, find the parent
// element using `findDOMNode`.
if (this.props.typeName === null) {
this.findDOMContainer();
}

// Run our `appearAnimation` if it was requested, right after the
// component mounts.
const shouldTriggerFLIP =
Expand Down Expand Up @@ -155,23 +159,8 @@ class FlipMove extends Component<ConvertedProps, FlipMoveState> {
}

componentDidUpdate(previousProps: ConvertedProps) {
// If wrapperless mode was activated, we need to make sure we still have a
// valid parentNode to properly animate.
// If no anchor was provided we fall back to using findDomNode.
if (!this.props.typeName) {
if (!this.props.delegated.anchor) {
// Render warning if wrapperless mode is activated but no anchor has
// been provided
this.logAnchorWarning();

this.parentData.domNode =
/* eslint-disable react/no-find-dom-node */
// $FlowFixMe we because we now parentNode has to be HTMLElement
ReactDOM.findDOMNode(this) && ReactDOM.findDOMNode(this).parentNode;
/* eslint-enable react/no-find-dom-node */
} else {
this.parentData.domNode = this.props.delegated.anchor;
}
if (this.props.typeName === null) {
this.findDOMContainer();
}
// If the children have been re-arranged, moved, or added/removed,
// trigger the main FLIP animation.
Expand All @@ -197,17 +186,17 @@ class FlipMove extends Component<ConvertedProps, FlipMoveState> {
}
}

logAnchorWarning = () => {
if (!this.wasWarned) {
console.warn(`
>> Error, via react-flip-move <<

Wrapperless mode was activated but no anchor has been provided to react-flip-move.

Please use either the 'typeName' prop to pass a wrapper type (such as 'ul') or make sure a valid HTMLElement is passed in 'anchor'
`);
this.wasWarned = true;
findDOMContainer = () => {
// eslint-disable-next-line react/no-find-dom-node
const domNode = ReactDOM.findDOMNode(this);
const parentNode = domNode && domNode.parentNode;

// This ought to be impossible, but handling it for Flow's sake.
if (!parentNode || !(parentNode instanceof HTMLElement)) {
return;
}

this.parentData.domNode = parentNode;
};

runAnimation = () => {
Expand Down
Loading