Skip to content

Commit

Permalink
Add onAnimationEnd prop (#198)
Browse files Browse the repository at this point in the history 
* support onAnimationEnd

* add to defaultProps
  • Loading branch information
@EyMaddis @iRoachie
EyMaddis authored and iRoachie committed May 27, 2018
1 parent ed6283a commit 3a600ca
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 1 deletion.
8 changes: 8 additions & 0 deletions Accordion.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export default class Accordion extends Component {
touchableProps: PropTypes.object,
disabled: PropTypes.bool,
expandFromBottom: PropTypes.bool,
onAnimationEnd: PropTypes.func,
};

static defaultProps = {
Expand All @@ -35,6 +36,7 @@ export default class Accordion extends Component {
expandFromBottom: false,
touchableComponent: TouchableHighlight,
renderSectionTitle: () => null,
onAnimationEnd: () => {},
};

constructor(props) {
Expand Down Expand Up @@ -96,6 +98,12 @@ export default class Accordion extends Component {
<Collapsible
collapsed={this.state.activeSection !== key}
{...collapsibleProps}
onAnimationEnd={() => {
const { onAnimationEnd } = this.props
if(onAnimationEnd) {
onAnimationEnd(section, key)
}
}}
>
{this.props.renderContent(
section,
Expand Down
4 changes: 3 additions & 1 deletion Collapsible.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export default class Collapsible extends Component {
duration: PropTypes.number,
easing: PropTypes.oneOfType([PropTypes.string, PropTypes.func]),
style: ViewPropTypes.style,
onAnimationEnd: PropTypes.func,
children: PropTypes.node,
};

Expand All @@ -22,6 +23,7 @@ export default class Collapsible extends Component {
collapsedHeight: 0,
duration: 300,
easing: 'easeOutCubic',
onAnimationEnd: () => {},
};

constructor(props) {
Expand Down Expand Up @@ -142,7 +144,7 @@ export default class Collapsible extends Component {
toValue: height,
duration,
easing,
}).start(() => this.setState({ animating: false }));
}).start(() => this.setState({ animating: false }, this.props.onAnimationEnd));

This comment has been minimized.

Sign in to view

Copy link
@slorber

slorber Jun 11, 2018

Contributor

hey, just wondering but isn't there a chance this callback gets fired even after component unmounts?

This comment has been minimized.

Sign in to view

Copy link
@iRoachie

iRoachie Jun 11, 2018

Collaborator

It's possible, unmounting a component doesn't stop its execution. Any update cycles will complete.

This comment has been minimized.

Sign in to view

Copy link
@slorber

slorber Jun 11, 2018

Contributor

maybe it would be better to ensure the component callback does not fire so? it's always strange from an user when this happens, as generally we end up with annoying react warnings or worse...

This comment has been minimized.

Sign in to view

Copy link
@iRoachie

iRoachie Jun 11, 2018

Collaborator

Maybe the developers can manage their unmounts after onAnimationEnd occurs? I don't see how I could manage this within the library itself.

This comment has been minimized.

Sign in to view

Copy link
@slorber

slorber Jun 11, 2018

Contributor

This can do the job:

componentWillUnmount() {
  this.unmounted = true;
}

const callback = () => !this.unmounted && this.props.onAnimationEnd()

This comment has been minimized.

Sign in to view

Copy link
@iRoachie

iRoachie Jun 11, 2018

Collaborator

this.props.onAnimationEnd should always be called though, even if the component was unmounted. If you're using the onAnimationEnd callback to set state in your own component then can't you apply the same example above?

This comment has been minimized.

Sign in to view

Copy link
@slorber

slorber Jun 11, 2018

Contributor

sure, I can definitively do this myself, however I find this is always more convenient to do it at the lib level and got annoying warnings in the past on such usecases. Can you think a single usecase where the user would want to be notified of the end of a js animation of a component that is not rendered anymore? I've never seen such usecase in the last 4 years using React

This comment has been minimized.

Sign in to view

Copy link
@iRoachie

iRoachie Jun 11, 2018

Collaborator

Theoretically, I'd have to add to this every setState call, since the component could be unmounted at any time. Is there any reason why in your case you are umounting the Collapsible between setState calls? Just trying to understand how the warning occurred in your case

This comment has been minimized.

Sign in to view

Copy link
@slorber

slorber Jun 11, 2018

Contributor

btw this.setState({ animating: false }) would also produce an error if firing after unmount.

The best solution is probably to call stop() on the animation on unmount.

It probably does not happen very frequently but it can if animation is slow

This comment has been minimized.

Sign in to view

Copy link
@slorber

slorber Jun 11, 2018

Contributor

I actually did not get any warning as my animation is fast, but it's indeed possible in my app for user to press things fast and navigate to another screen while the animation is still playing.

If you don't want to fix this potential issue, i'll still be fine, just wanted to say it may be good to fix in the future

This comment has been minimized.

Sign in to view

Copy link
@iRoachie

iRoachie Jun 11, 2018

Collaborator

Yea I get what you're saying, just was wondering if this could happen at any setState we have. If you're willing to make a PR for it I'd be more than willing to merge it in.

This comment has been minimized.

Sign in to view

Copy link
@slorber

slorber Jun 11, 2018

Contributor

sure i'll do one, not sure when yet but it's on my todo
}

_handleLayoutChange = event => {
Expand Down
2 changes: 2 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import Collapsible from 'react-native-collapsible';
| **`duration`** | Duration of transition in milliseconds | `300` |
| **`easing`** | Function or function name from [`Easing`](https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/Easing.js) (or [`tween-functions`](https://github.com/chenglou/tween-functions) if < RN 0.8). Collapsible will try to combine `Easing` functions for you if you name them like `tween-functions`. | `easeOutCubic` |
| **`style`** | Optional styling for the container | |
| **`onAnimationEnd`** | Callback when the toggle animation is done. Useful to avoid heavy layouting work during the animation | `() => {}` |

## Accordion Usage

Expand Down Expand Up @@ -62,6 +63,7 @@ import Accordion from 'react-native-collapsible/Accordion';
| **`align`** | See `Collapsible` |
| **`duration`** | See `Collapsible` |
| **`easing`** | See `Collapsible` |
| **`onAnimationEnd(key, index)`** | See `Collapsible`. |
| **`expandFromBottom`** | Expand content from the bottom instead of the top |

## Demo
Expand Down

0 comments on commit 3a600ca

Please sign in to comment.