fix(Popup) popup appearing in wrong position when open prop is set (#…

…2775) * fix(Popup) popup appearing in wrong position when open prop is set * fix(Popup): apply RAF to pin Popup to trigger component * fix(Popup): cancel animation on unMount
Semantic-Org · May 19, 2018 · 4f82da4 · 4f82da4
1 parent 0ea06ae
commit 4f82da4
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 34 deletions.
diff --git a/src/modules/Popup/Popup.js b/src/modules/Popup/Popup.js
@@ -19,6 +19,7 @@ import {
 import Portal from '../../addons/Portal'
 import PopupContent from './PopupContent'
 import PopupHeader from './PopupHeader'
+import Ref from '../../addons/Ref'
 
 const debug = makeDebugger('popup')
 
@@ -164,27 +165,28 @@ export default class Popup extends Component {
     const { pageYOffset, pageXOffset } = window
     const { clientWidth, clientHeight } = document.documentElement
 
+    const coords = this.coords || this.triggerRef.getBoundingClientRect()
     if (_.includes(positions, 'right')) {
-      style.right = Math.round(clientWidth - (this.coords.right + pageXOffset))
+      style.right = Math.round(clientWidth - (coords.right + pageXOffset))
       style.left = 'auto'
     } else if (_.includes(positions, 'left')) {
-      style.left = Math.round(this.coords.left + pageXOffset)
+      style.left = Math.round(coords.left + pageXOffset)
       style.right = 'auto'
     } else { // if not left nor right, we are horizontally centering the element
-      const xOffset = (this.coords.width - this.popupCoords.width) / 2
-      style.left = Math.round(this.coords.left + xOffset + pageXOffset)
+      const xOffset = (coords.width - this.popupCoords.width) / 2
+      style.left = Math.round(coords.left + xOffset + pageXOffset)
       style.right = 'auto'
     }
 
     if (_.includes(positions, 'top')) {
-      style.bottom = Math.round(clientHeight - (this.coords.top + pageYOffset))
+      style.bottom = Math.round(clientHeight - (coords.top + pageYOffset))
       style.top = 'auto'
     } else if (_.includes(positions, 'bottom')) {
-      style.top = Math.round(this.coords.bottom + pageYOffset)
+      style.top = Math.round(coords.bottom + pageYOffset)
       style.bottom = 'auto'
     } else { // if not top nor bottom, we are vertically centering the element
-      const yOffset = (this.coords.height + this.popupCoords.height) / 2
-      style.top = Math.round((this.coords.bottom + pageYOffset) - yOffset)
+      const yOffset = (coords.height + this.popupCoords.height) / 2
+      style.top = Math.round((coords.bottom + pageYOffset) - yOffset)
       style.bottom = 'auto'
 
       const xOffset = this.popupCoords.width + 8
@@ -246,7 +248,7 @@ export default class Popup extends Component {
   }
 
   setPopupStyle() {
-    if (!this.coords || !this.popupCoords) return
+    if ((!this.coords && !this.triggerRef) || !this.popupCoords) return
     let position = this.props.position
     let style = this.computePopupStyle(position)
     const { keepInViewPort } = this.props
@@ -324,13 +326,15 @@ export default class Popup extends Component {
     const { hideOnScroll } = this.props
 
     if (hideOnScroll) eventStack.sub('scroll', this.hideOnScroll, { target: window })
+    this.setPosition()
     _.invoke(this.props, 'onMount', e, this.props)
   }
 
   handlePortalUnmount = (e) => {
     debug('handlePortalUnmount()')
     const { hideOnScroll } = this.props
 
+    cancelAnimationFrame(this.animationRequestId)
     if (hideOnScroll) eventStack.unsub('scroll', this.hideOnScroll, { target: window })
     _.invoke(this.props, 'onUnmount', e, this.props)
   }
@@ -341,6 +345,21 @@ export default class Popup extends Component {
     this.setPopupStyle()
   }
 
+  handleTriggerRef = (triggerRef) => {
+    debug('triggerMounted()')
+    if (triggerRef) {
+      this.triggerRef = triggerRef
+    }
+  }
+
+  setPosition = () => {
+    if (this.triggerRef) {
+      this.setPopupStyle(this.props.position)
+    }
+
+    this.animationRequestId = requestAnimationFrame(this.setPosition)
+  }
+
   render() {
     const {
       basic,
@@ -394,16 +413,18 @@ export default class Popup extends Component {
     debug('portal props:', mergedPortalProps)
 
     return (
-      <Portal
-        {...mergedPortalProps}
-        trigger={trigger}
-        onClose={this.handleClose}
-        onMount={this.handlePortalMount}
-        onOpen={this.handleOpen}
-        onUnmount={this.handlePortalUnmount}
-      >
-        {popupJSX}
-      </Portal>
+      <Ref innerRef={this.handleTriggerRef}>
+        <Portal
+          {...mergedPortalProps}
+          trigger={trigger}
+          onClose={this.handleClose}
+          onMount={this.handlePortalMount}
+          onOpen={this.handleOpen}
+          onUnmount={this.handlePortalUnmount}
+        >
+          {popupJSX}
+        </Portal>
+      </Ref>
     )
   }
 }
diff --git a/test/specs/modules/Popup/Popup-test.js b/test/specs/modules/Popup/Popup-test.js
@@ -1,7 +1,7 @@
 import _ from 'lodash'
 import React from 'react'
 
-import Portal from 'src/addons/Portal/Portal'
+import Ref from 'src/addons/Ref/Ref'
 import { SUI } from 'src/lib'
 import Popup, { POSITIONS } from 'src/modules/Popup/Popup'
 import PopupHeader from 'src/modules/Popup/PopupHeader'
@@ -21,7 +21,10 @@ const wrapperShallow = (...args) => (wrapper = shallow(...args))
 
 const assertIn = (node, selector, isPresent = true) => {
   const didFind = node.querySelector(selector) !== null
-  didFind.should.equal(isPresent, `${didFind ? 'Found' : 'Did not find'} "${selector}" in the ${node}.`)
+  didFind.should.equal(
+    isPresent,
+    `${didFind ? 'Found' : 'Did not find'} "${selector}" in the ${node}.`,
+  )
 }
 const assertInBody = (...args) => assertIn(document.body, ...args)
 
@@ -48,7 +51,7 @@ describe('Popup', () => {
   it('renders a Portal', () => {
     wrapperShallow(<Popup />)
       .type()
-      .should.equal(Portal)
+      .should.equal(Ref)
   })
 
   it('renders to the document body', () => {
@@ -59,9 +62,7 @@ describe('Popup', () => {
   it('renders child text', () => {
     wrapperMount(<Popup open>child text</Popup>)
 
-    document.querySelector('.ui.popup.visible')
-      .innerText
-      .should.equal('child text')
+    document.querySelector('.ui.popup.visible').innerText.should.equal('child text')
   })
 
   it('renders child components', () => {
@@ -139,14 +140,9 @@ describe('Popup', () => {
 
   describe('position', () => {
     POSITIONS.forEach((position) => {
-      it('is always within the viewport', () => {
+      it('is always within the viewport when the trigger is clicked', () => {
         wrapperMount(
-          <Popup
-            content='_'
-            position={position}
-            trigger={<button>foo</button>}
-            on='click'
-          />,
+          <Popup content='_' position={position} trigger={<button>foo</button>} on='click' />,
         )
         wrapper.find('button').simulate('click')
 
@@ -158,6 +154,14 @@ describe('Popup', () => {
         expect(bottom).to.be.at.most(document.documentElement.clientHeight)
         expect(right).to.be.at.most(document.documentElement.clientWidth)
       })
+      it('is positioned properly when open property is set', () => {
+        wrapperMount(<Popup content='_' position={position} open trigger={<button>foo</button>} />)
+        const element = document.querySelector('.popup.ui')
+        element.style.should.not.have.property('top', '')
+        element.style.should.not.have.property('left', '')
+        element.style.should.not.have.property('bottom', '')
+        element.style.should.not.have.property('right', '')
+      })
       it('is the original if no horizontal position fits within the viewport', () => {
         wrapperMount(
           <Popup
@@ -304,8 +308,9 @@ describe('Popup', () => {
 
     it('it appears on multiple', (done) => {
       const trigger = <button>foo</button>
-      const button = wrapperMount(<Popup on={['click', 'hover']} content='foo' header='bar' trigger={trigger} />)
-        .find('button')
+      const button = wrapperMount(
+        <Popup on={['click', 'hover']} content='foo' header='bar' trigger={trigger} />,
+      ).find('button')
 
       button.simulate('click')
       assertInBody('.ui.popup.visible')