From c837d8db301ffbe669166c7bd07ec0556ec3dfe5 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Mon, 20 Jul 2015 03:12:37 -0400 Subject: [PATCH 1/6] [fixed] Only calculate overlay position on display Fixes #1018 --- src/Collapse.js | 72 +++++---- src/Fade.js | 68 ++++---- src/Overlay.js | 96 +++++------ src/Position.js | 93 ++++++----- src/Transition.js | 350 ++++++++++++++++++++--------------------- test/TransitionSpec.js | 52 ++---- 6 files changed, 349 insertions(+), 382 deletions(-) diff --git a/src/Collapse.js b/src/Collapse.js index 505c116085..299ec8a625 100644 --- a/src/Collapse.js +++ b/src/Collapse.js @@ -115,84 +115,82 @@ class Collapse extends React.Component { Collapse.propTypes = { /** - * Collapse the Component in or out. + * Whether the component is entered; triggers the enter or exit animation */ - in: React.PropTypes.bool, + in: React.PropTypes.bool, /** - * Provide the duration of the animation in milliseconds, used to ensure that finishing callbacks are fired even if the - * original browser transition end events are canceled. + * Whether the component should be unmounted (removed from DOM) when exited */ - duration: React.PropTypes.number, + unmountOnExit: React.PropTypes.bool, /** - * Specifies the dimension used when collapsing. - * - * _Note: Bootstrap only partially supports 'width'! - * You will need to supply your own css animation for the `.width` css class._ + * Whether transition in should run when the Transition component mounts, if + * the component is initially entered */ - dimension: React.PropTypes.oneOfType([ - React.PropTypes.oneOf(['height', 'width']), - React.PropTypes.func - ]), + transitionAppear: React.PropTypes.bool, /** - * A function that returns the height or width of the animating DOM node. Allows for providing some custom logic how much - * Collapse component should animate in its specified dimension. - * - * `getDimensionValue` is called with the current dimension prop value and the DOM node. + * Duration of the animation in milliseconds, to ensure that finishing + * callbacks are fired even if the original browser transition end events are + * canceled */ - getDimensionValue: React.PropTypes.func, + duration: React.PropTypes.number, /** - * A Callback fired before the component starts to expand. + * Callback fired before the "entering" classes are applied */ onEnter: React.PropTypes.func, - /** - * A Callback fired immediately after the component starts to expand. + * Callback fired after the "entering" classes are applied */ onEntering: React.PropTypes.func, - /** - * A Callback fired after the component has expanded. + * Callback fired after the "enter" classes are applied */ onEntered: React.PropTypes.func, - /** - * A Callback fired before the component starts to collapse. + * Callback fired before the "exiting" classes are applied */ onExit: React.PropTypes.func, - /** - * A Callback fired immediately after the component starts to collapse. + * Callback fired after the "exiting" classes are applied */ onExiting: React.PropTypes.func, - /** - * A Callback fired after the component has collapsed. + * Callback fired after the "exited" classes are applied */ onExited: React.PropTypes.func, /** - * Specify whether the transitioning component should be unmounted (removed from the DOM) once the exit animation finishes. + * The dimension used when collapsing + * + * _Note: Bootstrap only partially supports 'width'! + * You will need to supply your own CSS animation for the `.width` CSS class._ */ - unmountOnExit: React.PropTypes.bool, + dimension: React.PropTypes.oneOfType([ + React.PropTypes.oneOf(['height', 'width']), + React.PropTypes.func + ]), /** - * Specify whether the component should collapse or expand when it mounts. + * Function that returns the height or width of the animating DOM node + * + * Allows for providing some custom logic for how much the Collapse component + * should animate in its specified dimension. Called with the current + * dimension prop value and the DOM node. */ - transitionAppear: React.PropTypes.bool + getDimensionValue: React.PropTypes.func }; Collapse.defaultProps = { - in: false, + in: false, duration: 300, - dimension: 'height', - transitionAppear: false, unmountOnExit: false, + transitionAppear: false, + + dimension: 'height', getDimensionValue }; export default Collapse; - diff --git a/src/Fade.js b/src/Fade.js index 523508fff5..528e839d85 100644 --- a/src/Fade.js +++ b/src/Fade.js @@ -2,87 +2,77 @@ import React from 'react'; import Transition from './Transition'; class Fade extends React.Component { - - constructor(props, context){ - super(props, context); - } - render() { return ( - { this.props.children } + {this.props.children} ); } } +// Explicitly copied from Transition for doc generation. + Fade.propTypes = { /** - * Fade the Component in or out. + * Whether the component is entered; triggers the enter or exit animation */ - in: React.PropTypes.bool, + in: React.PropTypes.bool, /** - * Provide the duration of the animation in milliseconds, used to ensure that finishing callbacks are fired even if the - * original browser transition end events are canceled. + * Whether the component should be unmounted (removed from DOM) when exited */ - duration: React.PropTypes.number, + unmountOnExit: React.PropTypes.bool, /** - * A Callback fired before the component starts to fade in. + * Whether transition in should run when the Transition component mounts, if + * the component is initially entered */ - onEnter: React.PropTypes.func, + transitionAppear: React.PropTypes.bool, /** - * A Callback fired immediately after the component has started to faded in. + * Duration of the animation in milliseconds, to ensure that finishing + * callbacks are fired even if the original browser transition end events are + * canceled */ - onEntering: React.PropTypes.func, + duration: React.PropTypes.number, /** - * A Callback fired after the component has faded in. + * Callback fired before the "entering" classes are applied */ - onEntered: React.PropTypes.func, - + onEnter: React.PropTypes.func, /** - * A Callback fired before the component starts to fade out. + * Callback fired after the "entering" classes are applied */ - onExit: React.PropTypes.func, - + onEntering: React.PropTypes.func, /** - * A Callback fired immediately after the component has started to faded out. + * Callback fired after the "enter" classes are applied */ - onExiting: React.PropTypes.func, - + onEntered: React.PropTypes.func, /** - * A Callback fired after the component has faded out. + * Callback fired before the "exiting" classes are applied */ - onExited: React.PropTypes.func, - - + onExit: React.PropTypes.func, /** - * Specify whether the transitioning component should be unmounted (removed from the DOM) once the exit animation finishes. + * Callback fired after the "exiting" classes are applied */ - unmountOnExit: React.PropTypes.bool, - + onExiting: React.PropTypes.func, /** - * Specify whether the component should fade in or out when it mounts. + * Callback fired after the "exited" classes are applied */ - transitionAppear: React.PropTypes.bool - + onExited: React.PropTypes.func }; Fade.defaultProps = { - in: false, + in: false, duration: 300, - dimension: 'height', - transitionAppear: false, - unmountOnExit: false + unmountOnExit: false, + transitionAppear: false }; export default Fade; diff --git a/src/Overlay.js b/src/Overlay.js index 8e3e996e5b..0a4932903e 100644 --- a/src/Overlay.js +++ b/src/Overlay.js @@ -7,31 +7,24 @@ import CustomPropTypes from './utils/CustomPropTypes'; import Fade from './Fade'; import classNames from 'classnames'; - class Overlay extends React.Component { - - constructor(props, context){ + constructor(props, context) { super(props, context); - this.state = { exited: false }; + this.state = {exited: !props.show}; this.onHiddenListener = this.handleHidden.bind(this); } componentWillReceiveProps(nextProps) { - let state = {}; - - if ( !nextProps.show && this.props.show ){ - state.exiting = true; - } - if (nextProps.show) { - state = { exited: false, exiting: false }; + this.setState({exited: false}); + } else if (!nextProps.animation) { + // Otherwise let handleHidden take care of marking exited. + this.setState({exited: true}); } - - this.setState(state); } - render(){ + render() { let { container , containerPadding @@ -42,56 +35,63 @@ class Overlay extends React.Component { , animation: Transition , ...props } = this.props; - let child = null; - - if ( Transition === true ){ + if (Transition === true) { Transition = Fade; } - if (props.show || (Transition && this.state.exiting && !this.state.exited)) { + // Don't un-render the overlay while it's transitioning out. + const mountOverlay = props.show || (Transition && !this.state.exited); + + if (!mountOverlay) { + // Don't bother showing anything if we don't have to. + return null; + } - child = children; + let child = children; - // Position the child before the animation to avoid `null` DOM nodes + if (Transition) { + // This animates the child by injecting props, so it must be inner-most. child = ( - - { child } - + + {child} + + ); + } else { + child = cloneElement( + child, + {className: classNames('in', child.className)} ); - - child = Transition - ? ( - - { child } - - ) - : cloneElement(child, { className: classNames('in', child.className) }); - - //Adds a wrapping div so it cannot be before Transition - if (rootClose) { - child = ( - - { child } - - ); - } } + // This must wrap the transition to avoid position recalculations. + child = ( + + {child} + + ); + + // This goes after everything else because it adds a wrapping div. + if (rootClose) { + child = ( + + {child} + + ); + } return ( - { child } + {child} ); } - handleHidden(){ - this.setState({ exited: true, exiting: false }); + handleHidden() { + this.setState({exited: true}); } } diff --git a/src/Position.js b/src/Position.js index 6b3b8ff9a0..cb4408e041 100644 --- a/src/Position.js +++ b/src/Position.js @@ -4,9 +4,9 @@ import { calcOverlayPosition } from './utils/overlayPositionUtils'; import CustomPropTypes from './utils/CustomPropTypes'; class Position extends React.Component { - constructor(props, context){ super(props, context); + this.state = { positionLeft: null, positionTop: null, @@ -15,33 +15,35 @@ class Position extends React.Component { }; } - componentWillMount(){ - this._needsFlush = true; + componentDidMount() { + this.updatePosition(this.props, this.getTargetSafe(this.props)); } - componentWillReceiveProps(){ - this._needsFlush = true; - } + componentWillReceiveProps(nextProps) { + if (nextProps.target !== this.props.target) { + const target = this.getTargetSafe(this.props); + const nextTarget = this.getTargetSafe(nextProps); - componentDidMount(){ - this._maybeUpdatePosition(); - } - componentDidUpdate(){ - this._maybeUpdatePosition(); + if (nextTarget !== target) { + this.updatePosition(nextProps, nextTarget); + } + } } render() { - let { children, ...props } = this.props; - let { positionLeft, positionTop, ...arrows } = this.props.target ? this.state : {}; + const {children, ...props} = this.props; + const {positionLeft, positionTop, ...arrowPosition } = this.state; + const child = React.Children.only(children); return cloneElement( - React.Children.only(children), { + child, + { ...props, - ...arrows, + ...arrowPosition, positionTop, positionLeft, style: { - ...children.props.style, + ...child.props.style, left: positionLeft, top: positionTop } @@ -49,55 +51,62 @@ class Position extends React.Component { ); } - _maybeUpdatePosition(){ - if ( this._needsFlush ) { - this._needsFlush = false; - this._updatePosition(); + getTargetSafe(props) { + if (!props.target) { + return null; } + + return props.target(props); } - _updatePosition() { - if ( this.props.target == null ){ + updatePosition(props, target) { + if (!target) { + this.setState({ + positionLeft: null, + positionTop: null, + arrowOffsetLeft: null, + arrowOffsetTop: null + }); + return; } - let overlay = React.findDOMNode(this); - let target = React.findDOMNode(this.props.target(this.props)); - let container = React.findDOMNode(this.props.container) || domUtils.ownerDocument(this).body; - - this.setState( - calcOverlayPosition( - this.props.placement - , overlay - , target - , container - , this.props.containerPadding)); + const overlay = React.findDOMNode(this); + const container = + React.findDOMNode(props.container) || domUtils.ownerDocument(this).body; + + this.setState(calcOverlayPosition( + props.placement, + overlay, + target, + container, + props.containerPadding + )); } } Position.propTypes = { /** - * The target DOM node the Component is positioned next too. + * Function mapping props to DOM node the component is positioned next to */ - target: React.PropTypes.func, + target: React.PropTypes.func, /** - * The "offsetParent" of the Component + * "offsetParent" of the component */ - container: CustomPropTypes.mountable, + container: CustomPropTypes.mountable, /** - * Distance in pixels the Component should be positioned to the edge of the Container. + * Minimum spacing in pixels between container border and component border */ containerPadding: React.PropTypes.number, /** - * The location that the overlay should be positioned to its target. + * How to position the component relative to the target */ - placement: React.PropTypes.oneOf(['top', 'right', 'bottom', 'left']) + placement: React.PropTypes.oneOf(['top', 'right', 'bottom', 'left']) }; Position.defaultProps = { containerPadding: 0, - placement: 'right' + placement: 'right' }; - export default Position; diff --git a/src/Transition.js b/src/Transition.js index a49124aac7..85eb05c5f6 100644 --- a/src/Transition.js +++ b/src/Transition.js @@ -2,284 +2,274 @@ import React from 'react'; import TransitionEvents from './utils/TransitionEvents'; import classnames from 'classnames'; -function omit(obj, keys) { - let included = Object.keys(obj).filter( k => keys.indexOf(k) === -1); - let newObj = {}; - - included.forEach( key => newObj[key] = obj[key] ); - return newObj; -} - -function ensureTransitionEnd(node, handler, duration){ - let fired = false; - let done = e => { - if (!fired) { - fired = true; - handler(e); - } - }; - - if ( node ) { - TransitionEvents.addEndEventListener(node, done); - setTimeout(done, duration); - } else { - setTimeout(done, 0); - } -} - -// reading a dimension prop will cause the browser to recalculate, -// which will let our animations work -let triggerBrowserReflow = node => node.offsetHeight; //eslint-disable-line no-unused-expressions +const UNMOUNTED = 0; +export const EXITED = 1; +export const ENTERING = 2; +export const ENTERED = 3; +export const EXITING = 4; class Transition extends React.Component { - - constructor(props, context){ + constructor(props, context) { super(props, context); - this.state = { - in: !props.in, - transitioning: false - }; + let initialStatus; + if (props.in) { + // Perform enter in performNextTransition from componentDidMount. + initialStatus = props.transitionAppear ? EXITED : ENTERED; + } else { + initialStatus = props.unmountOnExit ? UNMOUNTED : EXITED; + } + this.state = {status: initialStatus}; - this.needsTransition = true; + this.nextCallback = null; } - componentWillReceiveProps(nextProps) { - if (nextProps.in !== this.props.in) { - this.needsTransition = true; - } + componentDidMount() { + this.performNextTransition(); } - componentDidUpdate() { - this.processChild(); - } + componentWillReceiveProps(nextProps) { + const status = this.state.status; + if (nextProps.in) { + if (status === EXITING) { + this.performEnter(nextProps); + } else if (this.props.unmountOnExit) { + // Perform enter in performNextTransition. + if (status === UNMOUNTED) { + this.setState({status: EXITED}); + } + } else if (status === EXITED) { + this.performEnter(nextProps); + } - componentWillMount() { - this._mounted = true; + // Otherwise we're already entering or entered. + } else { + if (status === ENTERING || status === ENTERED) { + this.performExit(nextProps); + } - if (!this.props.transitionAppear) { - this.needsTransition = false; - this.setState({ in: this.props.in }); + // Otherwise we're already exited or exiting. } } - componentWillUnmount(){ - this._mounted = false; + componentDidUpdate() { + this.performNextTransition(); } - componentDidMount() { - if (this.props.transitionAppear) { - this.processChild(); - } + componentWillUnmount() { + this.cancelNextCallback(); } - processChild(){ - let needsTransition = this.needsTransition; - let enter = this.props.in; - - if (needsTransition) { - this.needsTransition = false; - this[enter ? 'performEnter' : 'performLeave'](); + performNextTransition() { + if (this.state.status === EXITED) { + if (this.props.in) { + // Either because of transitionAppear or unmountOnExit. + this.performEnter(this.props); + } else if (this.props.unmountOnExit) { + this.setState({status: UNMOUNTED}); + } } } - performEnter() { - let maybeNode = React.findDOMNode(this); - - let enter = node => { - node = this.props.transitioningNode(node) || node; - - this.props.onEnter(node); + performEnter(props) { + this.cancelNextCallback(); + const node = React.findDOMNode(this); - this.safeSetState({ in: true, transitioning: true, needInitialRender: false }, ()=> { + // Not this.props, because we might be about to receive new props. + props.onEnter(node); - this.props.onEntering(node); + this.safeSetState({status: ENTERING}, () => { + this.props.onEntering(node); - ensureTransitionEnd(node, () => { - if ( this.state.in ){ - this.safeSetState({ - transitioning: false - }, () => this.props.onEntered(node)); - } - - }, this.props.duration); + this.onTransitionEnd(node, () => { + this.safeSetState({status: ENTERED}, () => { + this.props.onEntered(node); + }); }); - }; - - if (maybeNode) { - enter(maybeNode); - } - else if (this.props.unmountOnExit) { - this._ensureNode(enter); - } + }); } - performLeave() { - let node = React.findDOMNode(this); - - node = this.props.transitioningNode(node) || node; + performExit(props) { + this.cancelNextCallback(); + const node = React.findDOMNode(this); - this.props.onExit(node); + // Not this.props, because we might be about to receive new props. + props.onExit(node); - this.setState({ in: false, transitioning: true }, () => { + this.safeSetState({status: EXITING}, () => { this.props.onExiting(node); - ensureTransitionEnd(node, () => { - if ( !this.state.in ){ - this.safeSetState({ transitioning: false }, ()=> this.props.onExited(node)); - } - }, this.props.duration); + this.onTransitionEnd(node, () => { + this.safeSetState({status: EXITED}, () => { + this.props.onExited(node); + }); + }); }); } - _ensureNode(callback) { - - this.setState({ needInitialRender: true }, ()=> { - let node = React.findDOMNode(this); - - triggerBrowserReflow(node); - - callback(node); - }); + cancelNextCallback() { + if (this.nextCallback !== null) { + this.nextCallback.cancel(); + this.nextCallback = null; + } } - safeSetState(newState, cb){ - if (this._mounted) { - this.setState(newState, cb); - } + safeSetState(nextState, callback) { + // This shouldn't be necessary, but there are weird race conditions with + // setState callbacks and unmounting in testing, so always make sure that + // we can cancel any pending setState callbacks after we unmount. + this.setState(nextState, this.setNextCallback(callback)); } - render() { - let childProps = omit(this.props, Object.keys(Transition.propTypes).concat('children')); + setNextCallback(callback) { + let active = true; - let child = this.props.children; - let starting = this.state.needInitialRender; - let out = !this.state.in && !this.state.transitioning; + this.nextCallback = (event) => { + if (active) { + active = false; + this.nextCallback = null; - if ( !child || (this.props.unmountOnExit && out && !starting) ){ - return null; - } + callback(event); + } + }; - let classes = ''; + this.nextCallback.cancel = () => { + active = false; + }; - // using `classnames()` here causes a subtle bug, - // hence the verbose if/else if sequence. - if (this.state.in && !this.state.transitioning) { - classes = this.props.enteredClassName; - } + return this.nextCallback; + } + + onTransitionEnd(node, handler) { + this.setNextCallback(handler); - else if (this.state.in && this.state.transitioning) { - classes = this.props.enteringClassName; + if (node) { + TransitionEvents.addEndEventListener(node, this.nextCallback); + setTimeout(this.nextCallback, this.props.duration); + } else { + setTimeout(this.nextCallback, 0); } + } - else if (!this.state.in && !this.state.transitioning) { - classes = this.props.exitedClassName; + render() { + const status = this.state.status; + if (status === UNMOUNTED) { + return null; } - else if (!this.state.in && this.state.transitioning) { - classes = this.props.exitingClassName; + const {children, className, style, ...childProps} = this.props; + Object.keys(Transition.propTypes).forEach(key => delete childProps[key]); + + let transitionClassName; + if (status === EXITED) { + transitionClassName = this.props.exitedClassName; + } else if (status === ENTERING) { + transitionClassName = this.props.enteringClassName; + } else if (status === ENTERED) { + transitionClassName = this.props.enteredClassName; + } else if (status === EXITING) { + transitionClassName = this.props.exitingClassName; } - return React.cloneElement(child, { - ...childProps, - className: classnames( - child.props.className, - this.props.className, - classes) - }); + const child = React.Children.only(children); + return React.cloneElement( + child, + { + ...childProps, + className: classnames( + child.props.className, + className, + transitionClassName + ), + style: {...child.props.style, ...style} + } + ); } } Transition.propTypes = { /** - * Triggers the Enter or Exit animation + * Whether the component is entered; triggers the enter or exit animation */ - in: React.PropTypes.bool, + in: React.PropTypes.bool, /** - * Specify whether the transitioning component should be unmounted (removed from the DOM) once the exit animation finishes. + * Whether the component should be unmounted (removed from DOM) when exited */ - unmountOnExit: React.PropTypes.bool, + unmountOnExit: React.PropTypes.bool, /** - * Specify whether transitions should run when the Transition component mounts. + * Whether transition in should run when the Transition component mounts, if + * the component is initially entered */ transitionAppear: React.PropTypes.bool, /** - * Provide the duration of the animation in milliseconds, used to ensure that finishing callbacks are fired even if the - * original browser transition end events are canceled. + * Duration of the animation in milliseconds, to ensure that finishing + * callbacks are fired even if the original browser transition end events are + * canceled */ - duration: React.PropTypes.number, + duration: React.PropTypes.number, /** - * A css class or classes applied once the Component has exited. + * CSS class or classes applied when the component is exited */ - exitedClassName: React.PropTypes.string, + exitedClassName: React.PropTypes.string, /** - * A css class or classes applied while the Component is exiting. + * CSS class or classes applied while the component is exiting */ - exitingClassName: React.PropTypes.string, + exitingClassName: React.PropTypes.string, /** - * A css class or classes applied once the Component has entered. + * CSS class or classes applied when the component is entered */ - enteredClassName: React.PropTypes.string, + enteredClassName: React.PropTypes.string, /** - * A css class or classes applied while the Component is entering. + * CSS class or classes applied while the component is entering */ enteringClassName: React.PropTypes.string, /** - * A function that returns the DOM node to animate. This Node will have the transition classes applied to it. - * When left out, the Component will use its immediate child. - * - * @private - */ - transitioningNode: React.PropTypes.func, - - /** - * A callback fired just before the "entering" classes are applied + * Callback fired before the "entering" classes are applied */ - onEnter: React.PropTypes.func, + onEnter: React.PropTypes.func, /** - * A callback fired just after the "entering" classes are applied + * Callback fired after the "entering" classes are applied */ - onEntering: React.PropTypes.func, + onEntering: React.PropTypes.func, /** - * A callback fired after "enter" classes are applied + * Callback fired after the "enter" classes are applied */ - onEntered: React.PropTypes.func, + onEntered: React.PropTypes.func, /** - * A callback fired after "exiting" classes are applied + * Callback fired before the "exiting" classes are applied */ - onExit: React.PropTypes.func, + onExit: React.PropTypes.func, /** - * A callback fired after "exiting" classes are applied + * Callback fired after the "exiting" classes are applied */ - onExiting: React.PropTypes.func, + onExiting: React.PropTypes.func, /** - * A callback fired after "exit" classes are applied + * Callback fired after the "exited" classes are applied */ - onExited: React.PropTypes.func + onExited: React.PropTypes.func }; -// name the function so it is clearer in the documentation -const noop = ()=>{}; +// Name the function so it is clearer in the documentation +function noop() {} Transition.defaultProps = { - in: false, + in: false, duration: 300, unmountOnExit: false, transitionAppear: false, - transitioningNode: noop, - onEnter: noop, + onEnter: noop, onEntering: noop, - onEntered: noop, + onEntered: noop, - onExit: noop, - onExiting: noop, - onExited: noop + onExit: noop, + onExiting: noop, + onExited: noop }; export default Transition; diff --git a/test/TransitionSpec.js b/test/TransitionSpec.js index 7945ba8ceb..ced505b537 100644 --- a/test/TransitionSpec.js +++ b/test/TransitionSpec.js @@ -1,12 +1,10 @@ import React from 'react'; import ReactTestUtils from 'react/lib/ReactTestUtils'; import { render } from './helpers'; -import Transition from '../src/Transition'; -//import classNames from 'classnames'; +import Transition, {EXITED, ENTERING, ENTERED, EXITING} from + '../src/Transition'; describe('Transition', function () { - - it('should not transition on mount', function(){ let instance = render( { throw new Error('should not Enter'); }}> @@ -14,8 +12,7 @@ describe('Transition', function () { ); - instance.state.in.should.equal(true); - assert.ok(!instance.state.transitioning); + instance.state.status.should.equal(ENTERED); }); it('should transition on mount with transitionAppear', done =>{ @@ -28,8 +25,7 @@ describe('Transition', function () { ); - instance.state.in.should.equal(true); - instance.state.transitioning.should.equal(true); + instance.state.status.should.equal(EXITED); }); describe('entering', ()=> { @@ -51,10 +47,9 @@ describe('Transition', function () { let onEnter = sinon.spy(); let onEntering = sinon.spy(); - instance.state.in.should.equal(false); + instance.state.status.should.equal(EXITED); instance = instance.renderWithProps({ - in: true, onEnter, @@ -73,27 +68,23 @@ describe('Transition', function () { it('should move to each transition state', done => { let count = 0; - instance.state.in.should.equal(false); + instance.state.status.should.equal(EXITED); instance = instance.renderWithProps({ - in: true, onEnter(){ count++; - instance.state.in.should.equal(false); - instance.state.transitioning.should.equal(false); + instance.state.status.should.equal(EXITED); }, onEntering(){ count++; - instance.state.in.should.equal(true); - instance.state.transitioning.should.equal(true); + instance.state.status.should.equal(ENTERING); }, onEntered(){ - instance.state.in.should.equal(true); - instance.state.transitioning.should.equal(false); + instance.state.status.should.equal(ENTERED); assert.ok(count === 2); done(); } @@ -103,10 +94,9 @@ describe('Transition', function () { it('should apply classes at each transition state', done => { let count = 0; - instance.state.in.should.equal(false); + instance.state.status.should.equal(EXITED); instance = instance.renderWithProps({ - in: true, onEnter(node){ @@ -126,10 +116,8 @@ describe('Transition', function () { } }); }); - }); - describe('exiting', ()=> { let instance; @@ -150,10 +138,9 @@ describe('Transition', function () { let onExit = sinon.spy(); let onExiting = sinon.spy(); - instance.state.in.should.equal(true); + instance.state.status.should.equal(ENTERED); instance = instance.renderWithProps({ - in: false, onExit, @@ -172,27 +159,23 @@ describe('Transition', function () { it('should move to each transition state', done => { let count = 0; - instance.state.in.should.equal(true); + instance.state.status.should.equal(ENTERED); instance = instance.renderWithProps({ - in: false, onExit(){ count++; - instance.state.in.should.equal(true); - instance.state.transitioning.should.equal(false); + instance.state.status.should.equal(ENTERED); }, onExiting(){ count++; - instance.state.in.should.equal(false); - instance.state.transitioning.should.equal(true); + instance.state.status.should.equal(EXITING); }, onExited(){ - instance.state.in.should.equal(false); - instance.state.transitioning.should.equal(false); + instance.state.status.should.equal(EXITED); //assert.ok(count === 2); done(); } @@ -202,10 +185,9 @@ describe('Transition', function () { it('should apply classes at each transition state', done => { let count = 0; - instance.state.in.should.equal(true); + instance.state.status.should.equal(ENTERED); instance = instance.renderWithProps({ - in: false, onExit(node){ @@ -225,7 +207,5 @@ describe('Transition', function () { } }); }); - }); - }); From dbacfd9d95f54647e8f4abc4172f6141c83d98e1 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Mon, 20 Jul 2015 12:54:40 -0400 Subject: [PATCH 2/6] !fixup fixes to previous commit --- src/Collapse.js | 32 ++++++++++++++++++-------------- src/Fade.js | 23 ++++++++++++----------- src/Overlay.js | 19 ++++++++++--------- src/Transition.js | 36 +++++++++++++++++------------------- 4 files changed, 57 insertions(+), 53 deletions(-) diff --git a/src/Collapse.js b/src/Collapse.js index 299ec8a625..30f0cbaa32 100644 --- a/src/Collapse.js +++ b/src/Collapse.js @@ -113,57 +113,61 @@ class Collapse extends React.Component { } } +// Explicitly copied from Transition for doc generation. +// TODO: Remove duplication once #977 is resolved. + Collapse.propTypes = { /** - * Whether the component is entered; triggers the enter or exit animation + * Whether the component is expanded */ in: React.PropTypes.bool, /** - * Whether the component should be unmounted (removed from DOM) when exited + * Whether the component should be unmounted (removed from DOM) when + * collapsed */ unmountOnExit: React.PropTypes.bool, /** - * Whether transition in should run when the Transition component mounts, if - * the component is initially entered + * Whether the component should expand after mounting */ transitionAppear: React.PropTypes.bool, /** - * Duration of the animation in milliseconds, to ensure that finishing - * callbacks are fired even if the original browser transition end events are - * canceled + * Duration of the collapse animation in milliseconds, to ensure that + * finishing callbacks are fired even if the original browser transition end + * events are canceled */ duration: React.PropTypes.number, /** - * Callback fired before the "entering" classes are applied + * Callback fired before the component expands */ onEnter: React.PropTypes.func, /** - * Callback fired after the "entering" classes are applied + * Callback fired after the component starts to expand */ onEntering: React.PropTypes.func, /** - * Callback fired after the "enter" classes are applied + * Callback fired after the component has expanded */ onEntered: React.PropTypes.func, /** - * Callback fired before the "exiting" classes are applied + * Callback fired before the component collapses */ onExit: React.PropTypes.func, /** - * Callback fired after the "exiting" classes are applied + * Callback fired after the component starts to collapse */ onExiting: React.PropTypes.func, /** - * Callback fired after the "exited" classes are applied + * Callback fired after the component has collapsed */ onExited: React.PropTypes.func, /** - * The dimension used when collapsing + * The dimension used when collapsing, or a function that returns the + * dimension * * _Note: Bootstrap only partially supports 'width'! * You will need to supply your own CSS animation for the `.width` CSS class._ diff --git a/src/Fade.js b/src/Fade.js index 528e839d85..9e8df40b19 100644 --- a/src/Fade.js +++ b/src/Fade.js @@ -17,53 +17,54 @@ class Fade extends React.Component { } // Explicitly copied from Transition for doc generation. +// TODO: Remove duplication once #977 is resolved. Fade.propTypes = { /** - * Whether the component is entered; triggers the enter or exit animation + * Whether the component is faded in */ in: React.PropTypes.bool, /** - * Whether the component should be unmounted (removed from DOM) when exited + * Whether the component should be unmounted (removed from DOM) when faded + * out */ unmountOnExit: React.PropTypes.bool, /** - * Whether transition in should run when the Transition component mounts, if - * the component is initially entered + * Whether the component should fade in after mounting */ transitionAppear: React.PropTypes.bool, /** - * Duration of the animation in milliseconds, to ensure that finishing + * Duration of the fade animation in milliseconds, to ensure that finishing * callbacks are fired even if the original browser transition end events are * canceled */ duration: React.PropTypes.number, /** - * Callback fired before the "entering" classes are applied + * Callback fired before the component fades in */ onEnter: React.PropTypes.func, /** - * Callback fired after the "entering" classes are applied + * Callback fired after the component starts to fade in */ onEntering: React.PropTypes.func, /** - * Callback fired after the "enter" classes are applied + * Callback fired after the has component faded in */ onEntered: React.PropTypes.func, /** - * Callback fired before the "exiting" classes are applied + * Callback fired before the component fades out */ onExit: React.PropTypes.func, /** - * Callback fired after the "exiting" classes are applied + * Callback fired after the component starts to fade out */ onExiting: React.PropTypes.func, /** - * Callback fired after the "exited" classes are applied + * Callback fired after the component has faded out */ onExited: React.PropTypes.func }; diff --git a/src/Overlay.js b/src/Overlay.js index 0a4932903e..6d82a7f401 100644 --- a/src/Overlay.js +++ b/src/Overlay.js @@ -41,7 +41,6 @@ class Overlay extends React.Component { // Don't un-render the overlay while it's transitioning out. const mountOverlay = props.show || (Transition && !this.state.exited); - if (!mountOverlay) { // Don't bother showing anything if we don't have to. return null; @@ -49,8 +48,17 @@ class Overlay extends React.Component { let child = children; + // Position is be inner-most because it adds inline styles into the child, + // which the other wrappers don't forward correctly. + child = ( + + {child} + + ); + if (Transition) { - // This animates the child by injecting props, so it must be inner-most. + // This animates the child node by injecting props, so it must precede + // anything that adds a wrapping div. child = ( - {child} - - ); - // This goes after everything else because it adds a wrapping div. if (rootClose) { child = ( diff --git a/src/Transition.js b/src/Transition.js index 85eb05c5f6..260628997a 100644 --- a/src/Transition.js +++ b/src/Transition.js @@ -14,7 +14,7 @@ class Transition extends React.Component { let initialStatus; if (props.in) { - // Perform enter in performNextTransition from componentDidMount. + // Start enter transition in componentDidMount. initialStatus = props.transitionAppear ? EXITED : ENTERED; } else { initialStatus = props.unmountOnExit ? UNMOUNTED : EXITED; @@ -25,7 +25,9 @@ class Transition extends React.Component { } componentDidMount() { - this.performNextTransition(); + if (this.props.transitionAppear && this.props.in) { + this.performEnter(this.props); + } } componentWillReceiveProps(nextProps) { @@ -34,8 +36,8 @@ class Transition extends React.Component { if (status === EXITING) { this.performEnter(nextProps); } else if (this.props.unmountOnExit) { - // Perform enter in performNextTransition. if (status === UNMOUNTED) { + // Start enter transition in componentDidUpdate. this.setState({status: EXITED}); } } else if (status === EXITED) { @@ -53,24 +55,21 @@ class Transition extends React.Component { } componentDidUpdate() { - this.performNextTransition(); - } - - componentWillUnmount() { - this.cancelNextCallback(); - } - - performNextTransition() { - if (this.state.status === EXITED) { + if (this.props.unmountOnExit && this.state.status === EXITED) { + // EXITED is always a transitional state to either ENTERING or UNMOUNTED + // when using unmountOnExit. if (this.props.in) { - // Either because of transitionAppear or unmountOnExit. this.performEnter(this.props); - } else if (this.props.unmountOnExit) { + } else { this.setState({status: UNMOUNTED}); } } } + componentWillUnmount() { + this.cancelNextCallback(); + } + performEnter(props) { this.cancelNextCallback(); const node = React.findDOMNode(this); @@ -157,7 +156,7 @@ class Transition extends React.Component { return null; } - const {children, className, style, ...childProps} = this.props; + const {children, className, ...childProps} = this.props; Object.keys(Transition.propTypes).forEach(key => delete childProps[key]); let transitionClassName; @@ -180,8 +179,7 @@ class Transition extends React.Component { child.props.className, className, transitionClassName - ), - style: {...child.props.style, ...style} + ) } ); } @@ -189,7 +187,7 @@ class Transition extends React.Component { Transition.propTypes = { /** - * Whether the component is entered; triggers the enter or exit animation + * Whether the component is shown; triggers the enter or exit animation */ in: React.PropTypes.bool, @@ -200,7 +198,7 @@ Transition.propTypes = { /** * Whether transition in should run when the Transition component mounts, if - * the component is initially entered + * the component is initially shown */ transitionAppear: React.PropTypes.bool, From afe9d9c255840c2f87034e7cb18656420074bf73 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Mon, 20 Jul 2015 13:37:32 -0400 Subject: [PATCH 3/6] !fixup add unmountOnExit test cases --- src/Transition.js | 2 +- test/TransitionSpec.js | 130 +++++++++++++++++++++++++++++++---------- 2 files changed, 100 insertions(+), 32 deletions(-) diff --git a/src/Transition.js b/src/Transition.js index 260628997a..f1c557f4ed 100644 --- a/src/Transition.js +++ b/src/Transition.js @@ -2,7 +2,7 @@ import React from 'react'; import TransitionEvents from './utils/TransitionEvents'; import classnames from 'classnames'; -const UNMOUNTED = 0; +export const UNMOUNTED = 0; export const EXITED = 1; export const ENTERING = 2; export const ENTERED = 3; diff --git a/test/TransitionSpec.js b/test/TransitionSpec.js index ced505b537..1592e646f7 100644 --- a/test/TransitionSpec.js +++ b/test/TransitionSpec.js @@ -1,7 +1,7 @@ import React from 'react'; import ReactTestUtils from 'react/lib/ReactTestUtils'; import { render } from './helpers'; -import Transition, {EXITED, ENTERING, ENTERED, EXITING} from +import Transition, {UNMOUNTED, EXITED, ENTERING, ENTERED, EXITING} from '../src/Transition'; describe('Transition', function () { @@ -12,7 +12,7 @@ describe('Transition', function () { ); - instance.state.status.should.equal(ENTERED); + expect(instance.state.status).to.equal(ENTERED); }); it('should transition on mount with transitionAppear', done =>{ @@ -25,7 +25,7 @@ describe('Transition', function () { ); - instance.state.status.should.equal(EXITED); + expect(instance.state.status).to.equal(EXITED); }); describe('entering', ()=> { @@ -47,7 +47,7 @@ describe('Transition', function () { let onEnter = sinon.spy(); let onEntering = sinon.spy(); - instance.state.status.should.equal(EXITED); + expect(instance.state.status).to.equal(EXITED); instance = instance.renderWithProps({ in: true, @@ -57,9 +57,9 @@ describe('Transition', function () { onEntering, onEntered(){ - assert.ok(onEnter.calledOnce); - assert.ok(onEntering.calledOnce); - assert.ok(onEnter.calledBefore(onEntering)); + expect(onEnter.calledOnce).to.be.ok; + expect(onEntering.calledOnce).to.be.ok; + expect(onEnter.calledBefore(onEntering)).to.be.ok; done(); } }); @@ -68,24 +68,24 @@ describe('Transition', function () { it('should move to each transition state', done => { let count = 0; - instance.state.status.should.equal(EXITED); + expect(instance.state.status).to.equal(EXITED); instance = instance.renderWithProps({ in: true, onEnter(){ count++; - instance.state.status.should.equal(EXITED); + expect(instance.state.status).to.equal(EXITED); }, onEntering(){ count++; - instance.state.status.should.equal(ENTERING); + expect(instance.state.status).to.equal(ENTERING); }, onEntered(){ - instance.state.status.should.equal(ENTERED); - assert.ok(count === 2); + expect(instance.state.status).to.equal(ENTERED); + expect(count).to.equal(2); done(); } }); @@ -94,24 +94,24 @@ describe('Transition', function () { it('should apply classes at each transition state', done => { let count = 0; - instance.state.status.should.equal(EXITED); + expect(instance.state.status).to.equal(EXITED); instance = instance.renderWithProps({ in: true, onEnter(node){ count++; - assert.equal(node.className, ''); + expect(node.className).to.equal(''); }, onEntering(node){ count++; - assert.equal(node.className, 'test-entering'); + expect(node.className).to.equal('test-entering'); }, onEntered(node){ - assert.equal(node.className, 'test-enter'); - assert.ok(count === 2); + expect(node.className).to.equal('test-enter'); + expect(count).to.equal(2); done(); } }); @@ -138,7 +138,7 @@ describe('Transition', function () { let onExit = sinon.spy(); let onExiting = sinon.spy(); - instance.state.status.should.equal(ENTERED); + expect(instance.state.status).to.equal(ENTERED); instance = instance.renderWithProps({ in: false, @@ -148,9 +148,9 @@ describe('Transition', function () { onExiting, onExited(){ - assert.ok(onExit.calledOnce); - assert.ok(onExiting.calledOnce); - assert.ok(onExit.calledBefore(onExiting)); + expect(onExit.calledOnce).to.be.ok; + expect(onExiting.calledOnce).to.be.ok; + expect(onExit.calledBefore(onExiting)).to.be.ok; done(); } }); @@ -159,24 +159,24 @@ describe('Transition', function () { it('should move to each transition state', done => { let count = 0; - instance.state.status.should.equal(ENTERED); + expect(instance.state.status).to.equal(ENTERED); instance = instance.renderWithProps({ in: false, onExit(){ count++; - instance.state.status.should.equal(ENTERED); + expect(instance.state.status).to.equal(ENTERED); }, onExiting(){ count++; - instance.state.status.should.equal(EXITING); + expect(instance.state.status).to.equal(EXITING); }, onExited(){ - instance.state.status.should.equal(EXITED); - //assert.ok(count === 2); + expect(instance.state.status).to.equal(EXITED); + expect(count).to.equal(2); done(); } }); @@ -185,27 +185,95 @@ describe('Transition', function () { it('should apply classes at each transition state', done => { let count = 0; - instance.state.status.should.equal(ENTERED); + expect(instance.state.status).to.equal(ENTERED); instance = instance.renderWithProps({ in: false, onExit(node){ count++; - assert.equal(node.className, ''); + expect(node.className).to.equal(''); }, onExiting(node){ count++; - assert.equal(node.className, 'test-exiting'); + expect(node.className).to.equal('test-exiting'); }, onExited(node){ - assert.equal(node.className, 'test-exit'); - assert.ok(count === 2); + expect(node.className).to.equal('test-exit'); + expect(count).to.equal(2); done(); } }); }); }); + + describe('unmountOnExit', () => { + class UnmountTransition extends React.Component { + constructor(props) { + super(props); + + this.state = {in: props.initialIn}; + } + + render() { + return ( + +
+ + ); + } + + getStatus() { + return this.refs.transition.state.status; + } + } + + it('should mount when entering', done => { + const instance = render( + { + expect(instance.getStatus()).to.equal(EXITED); + expect(React.findDOMNode(instance)).to.exist; + + done(); + }} + /> + ); + + expect(instance.getStatus()).to.equal(UNMOUNTED); + expect(React.findDOMNode(instance)).to.not.exist; + + instance.setState({in: true}); + }); + + it('should unmount after exiting', done => { + const instance = render( + { + expect(instance.getStatus()).to.equal(UNMOUNTED); + expect(React.findDOMNode(instance)).to.not.exist; + + done(); + }} + /> + ); + + expect(instance.getStatus()).to.equal(ENTERED); + expect(React.findDOMNode(instance)).to.exist; + + instance.setState({in: false}); + }); + }); }); From 3a49843150df1451d57b4415497a66277dbb53f1 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Tue, 21 Jul 2015 10:18:36 -0400 Subject: [PATCH 4/6] fixup! reword docs for boolean fields --- src/Collapse.js | 8 ++++---- src/Fade.js | 8 ++++---- src/Transition.js | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/Collapse.js b/src/Collapse.js index 30f0cbaa32..ddca971770 100644 --- a/src/Collapse.js +++ b/src/Collapse.js @@ -118,18 +118,18 @@ class Collapse extends React.Component { Collapse.propTypes = { /** - * Whether the component is expanded + * Show the component; triggers the expand or collapse animation */ in: React.PropTypes.bool, /** - * Whether the component should be unmounted (removed from DOM) when - * collapsed + * Unmount the component (remove it from the DOM) when it is collapsed */ unmountOnExit: React.PropTypes.bool, /** - * Whether the component should expand after mounting + * Run the expand animation when the component mounts, if it is initially + * shown */ transitionAppear: React.PropTypes.bool, diff --git a/src/Fade.js b/src/Fade.js index 9e8df40b19..ce5a9a3370 100644 --- a/src/Fade.js +++ b/src/Fade.js @@ -21,18 +21,18 @@ class Fade extends React.Component { Fade.propTypes = { /** - * Whether the component is faded in + * Show the component; triggers the fade in or fade out animation */ in: React.PropTypes.bool, /** - * Whether the component should be unmounted (removed from DOM) when faded - * out + * Unmount the component (remove it from the DOM) when it is faded out */ unmountOnExit: React.PropTypes.bool, /** - * Whether the component should fade in after mounting + * Run the fade in animation when the component mounts, if it is initially + * shown */ transitionAppear: React.PropTypes.bool, diff --git a/src/Transition.js b/src/Transition.js index f1c557f4ed..8235d41f9f 100644 --- a/src/Transition.js +++ b/src/Transition.js @@ -187,18 +187,18 @@ class Transition extends React.Component { Transition.propTypes = { /** - * Whether the component is shown; triggers the enter or exit animation + * Show the component; triggers the enter or exit animation */ in: React.PropTypes.bool, /** - * Whether the component should be unmounted (removed from DOM) when exited + * Unmount the component (remove it from the DOM) when it is not shown */ unmountOnExit: React.PropTypes.bool, /** - * Whether transition in should run when the Transition component mounts, if - * the component is initially shown + * Run the enter animation when the component mounts, if it is initially + * shown */ transitionAppear: React.PropTypes.bool, From 6e194ead95f3f7a55c53a138acc3066f86969db0 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Tue, 21 Jul 2015 11:36:51 -0400 Subject: [PATCH 5/6] fixup! fix target change handler --- src/Position.js | 33 +++++++++------- test/PositionSpec.js | 90 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 13 deletions(-) diff --git a/src/Position.js b/src/Position.js index cb4408e041..c717d288ea 100644 --- a/src/Position.js +++ b/src/Position.js @@ -13,20 +13,24 @@ class Position extends React.Component { arrowOffsetLeft: null, arrowOffsetTop: null }; + + this._needsFlush = false; } componentDidMount() { - this.updatePosition(this.props, this.getTargetSafe(this.props)); + this.updatePosition(); } componentWillReceiveProps(nextProps) { if (nextProps.target !== this.props.target) { - const target = this.getTargetSafe(this.props); - const nextTarget = this.getTargetSafe(nextProps); + this._needsFlush = true; + } + } - if (nextTarget !== target) { - this.updatePosition(nextProps, nextTarget); - } + componentDidUpdate() { + if (this._needsFlush) { + this._needsFlush = false; + this.updatePosition(); } } @@ -51,15 +55,17 @@ class Position extends React.Component { ); } - getTargetSafe(props) { - if (!props.target) { + getTargetSafe() { + if (!this.props.target) { return null; } - return props.target(props); + return this.props.target(this.props); } - updatePosition(props, target) { + updatePosition() { + const target = this.getTargetSafe(); + if (!target) { this.setState({ positionLeft: null, @@ -73,14 +79,15 @@ class Position extends React.Component { const overlay = React.findDOMNode(this); const container = - React.findDOMNode(props.container) || domUtils.ownerDocument(this).body; + React.findDOMNode(this.props.container) || + domUtils.ownerDocument(this).body; this.setState(calcOverlayPosition( - props.placement, + this.props.placement, overlay, target, container, - props.containerPadding + this.props.containerPadding )); } } diff --git a/test/PositionSpec.js b/test/PositionSpec.js index 963b2c24b7..f157e1e0ee 100644 --- a/test/PositionSpec.js +++ b/test/PositionSpec.js @@ -1,6 +1,7 @@ import React from 'react'; import ReactTestUtils from 'react/lib/ReactTestUtils'; import Position from '../src/Position'; +import overlayPositionUtils from '../src/utils/overlayPositionUtils'; describe('Position', function () { it('Should output a child', function () { @@ -23,5 +24,94 @@ describe('Position', function () { }).to.throw(Error, /onlyChild must be passed a children with exactly one child/); }); + describe('position recalculation', function () { + beforeEach(function () { + sinon.spy(overlayPositionUtils, 'calcOverlayPosition'); + sinon.spy(Position.prototype, 'render'); + }); + + afterEach(function () { + overlayPositionUtils.calcOverlayPosition.restore(); + Position.prototype.render.restore(); + }); + + it('should recalculate when target changes', function () { + class TargetChanger extends React.Component { + constructor(props) { + super(props); + + this.state = {target: 'foo'}; + } + + render() { + return ( +
+ {this.renderTarget()} + + this.refs[this.state.target]}> +
+ +
+ ); + } + + renderTarget() { + if (this.state.target === 'foo') { + return (
); + } else if (this.state.target === 'bar') { + return (
); + } + } + } + + const instance = ReactTestUtils.renderIntoDocument(); + + expect(overlayPositionUtils.calcOverlayPosition).to.have.been.calledOnce; + + instance.setState({target: 'bar'}); + + expect(overlayPositionUtils.calcOverlayPosition) + .to.have.been.calledTwice; + }); + + it('should not recalculate when target doesn\'t change', function () { + class FooChanger extends React.Component { + constructor(props) { + super(props); + + this.getTarget = this.getTarget.bind(this); + + this.state = {foo: 0}; + } + + getTarget() { + return this.refs.target; + } + + render() { + return ( +
+
+ + +
+ +
+ ); + } + } + + const instance = ReactTestUtils.renderIntoDocument(); + + expect(overlayPositionUtils.calcOverlayPosition).to.have.been.calledOnce; + expect(Position.prototype.render).to.have.been.calledTwice; + + instance.setState({foo: 1}); + + expect(overlayPositionUtils.calcOverlayPosition).to.have.been.calledOnce; + expect(Position.prototype.render).to.have.been.calledThrice; + }); + }); + // ToDo: add remaining tests }); From 7836ba0176c8b728fdb25ffb86d6a34af566f60e Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Tue, 21 Jul 2015 12:41:38 -0400 Subject: [PATCH 6/6] fixup! less dumb check for target change --- src/Position.js | 29 ++++++++++++---- test/PositionSpec.js | 78 +++++++++++++++----------------------------- 2 files changed, 49 insertions(+), 58 deletions(-) diff --git a/src/Position.js b/src/Position.js index c717d288ea..ad9563a880 100644 --- a/src/Position.js +++ b/src/Position.js @@ -4,7 +4,7 @@ import { calcOverlayPosition } from './utils/overlayPositionUtils'; import CustomPropTypes from './utils/CustomPropTypes'; class Position extends React.Component { - constructor(props, context){ + constructor(props, context) { super(props, context); this.state = { @@ -15,16 +15,15 @@ class Position extends React.Component { }; this._needsFlush = false; + this._lastTarget = null; } componentDidMount() { this.updatePosition(); } - componentWillReceiveProps(nextProps) { - if (nextProps.target !== this.props.target) { - this._needsFlush = true; - } + componentWillReceiveProps() { + this._needsFlush = true; } componentDidUpdate() { @@ -34,9 +33,15 @@ class Position extends React.Component { } } + componentWillUnmount() { + // Probably not necessary, but just in case holding a reference to the + // target causes problems somewhere. + this._lastTarget = null; + } + render() { const {children, ...props} = this.props; - const {positionLeft, positionTop, ...arrowPosition } = this.state; + const {positionLeft, positionTop, ...arrowPosition} = this.state; const child = React.Children.only(children); return cloneElement( @@ -60,11 +65,21 @@ class Position extends React.Component { return null; } - return this.props.target(this.props); + const target = this.props.target(this.props); + if (!target) { + // This is so we can just use === check below on all falsy targets. + return null; + } + + return target; } updatePosition() { const target = this.getTargetSafe(); + if (target === this._lastTarget) { + return; + } + this._lastTarget = target; if (!target) { this.setState({ diff --git a/test/PositionSpec.js b/test/PositionSpec.js index f157e1e0ee..33283927b3 100644 --- a/test/PositionSpec.js +++ b/test/PositionSpec.js @@ -27,89 +27,65 @@ describe('Position', function () { describe('position recalculation', function () { beforeEach(function () { sinon.spy(overlayPositionUtils, 'calcOverlayPosition'); - sinon.spy(Position.prototype, 'render'); + sinon.spy(Position.prototype, 'componentWillReceiveProps'); }); afterEach(function () { overlayPositionUtils.calcOverlayPosition.restore(); - Position.prototype.render.restore(); + Position.prototype.componentWillReceiveProps.restore(); }); - it('should recalculate when target changes', function () { + it('Should only recalculate when target changes', function () { class TargetChanger extends React.Component { constructor(props) { super(props); - this.state = {target: 'foo'}; + this.state = { + target: 'foo', + fakeProp: 0 + }; } render() { return (
- {this.renderTarget()} +
+
- this.refs[this.state.target]}> + this.refs[this.state.target]} + fakeProp={this.state.fakeProp} + >
); } - - renderTarget() { - if (this.state.target === 'foo') { - return (
); - } else if (this.state.target === 'bar') { - return (
); - } - } } const instance = ReactTestUtils.renderIntoDocument(); - expect(overlayPositionUtils.calcOverlayPosition).to.have.been.calledOnce; + // Position calculates initial position. + expect(Position.prototype.componentWillReceiveProps) + .to.have.not.been.called; + expect(overlayPositionUtils.calcOverlayPosition) + .to.have.been.calledOnce; instance.setState({target: 'bar'}); + // Position receives new props and recalculates position. + expect(Position.prototype.componentWillReceiveProps) + .to.have.been.calledOnce; expect(overlayPositionUtils.calcOverlayPosition) .to.have.been.calledTwice; - }); - - it('should not recalculate when target doesn\'t change', function () { - class FooChanger extends React.Component { - constructor(props) { - super(props); - this.getTarget = this.getTarget.bind(this); + instance.setState({fakeProp: 1}); - this.state = {foo: 0}; - } - - getTarget() { - return this.refs.target; - } - - render() { - return ( -
-
- - -
- -
- ); - } - } - - const instance = ReactTestUtils.renderIntoDocument(); - - expect(overlayPositionUtils.calcOverlayPosition).to.have.been.calledOnce; - expect(Position.prototype.render).to.have.been.calledTwice; - - instance.setState({foo: 1}); - - expect(overlayPositionUtils.calcOverlayPosition).to.have.been.calledOnce; - expect(Position.prototype.render).to.have.been.calledThrice; + // Position receives new props but should not recalculate position. + expect(Position.prototype.componentWillReceiveProps) + .to.have.been.calledTwice; + expect(overlayPositionUtils.calcOverlayPosition) + .to.have.been.calledTwice; }); });