fixup! less dumb check for target change

michelgb · Jul 21, 2015 · 7836ba0 · 7836ba0
1 parent 6e194ea
commit 7836ba0
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 58 deletions.
diff --git 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
@@ -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 (
             <div>
-              {this.renderTarget()}
+              <div ref="foo" />
+              <div ref="bar" />
 
-              <Position target={() => this.refs[this.state.target]}>
+              <Position
+                target={() => this.refs[this.state.target]}
+                fakeProp={this.state.fakeProp}
+              >
                 <div />
               </Position>
             </div>
           );
         }
-
-        renderTarget() {
-          if (this.state.target === 'foo') {
-            return (<div ref="foo" style={{width: 100}} />);
-          } else if (this.state.target === 'bar') {
-            return (<div ref="bar" style={{width: 200}} />);
-          }
-        }
       }
 
       const instance = ReactTestUtils.renderIntoDocument(<TargetChanger />);
 
-      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 (
-            <div>
-              <div ref="target" style={{width: 100}} />
-
-              <Position target={this.getTarget} foo={this.state.foo}>
-                <div target="positioned" />
-              </Position>
-            </div>
-          );
-        }
-      }
-
-      const instance = ReactTestUtils.renderIntoDocument(<FooChanger />);
-
-      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;
     });
   });