From 4f82da4e6308629019a3a36ab976330290e68d2d Mon Sep 17 00:00:00 2001 From: Christopher Dieringer Date: Fri, 18 May 2018 17:12:24 -0700 Subject: [PATCH] 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 --- src/modules/Popup/Popup.js | 59 +++++++++++++++++--------- test/specs/modules/Popup/Popup-test.js | 35 ++++++++------- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/src/modules/Popup/Popup.js b/src/modules/Popup/Popup.js index 6d910e2f93..e7900addf8 100644 --- 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,6 +326,7 @@ 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) } @@ -331,6 +334,7 @@ export default class Popup extends Component { 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 ( - - {popupJSX} - + + + {popupJSX} + + ) } } diff --git a/test/specs/modules/Popup/Popup-test.js b/test/specs/modules/Popup/Popup-test.js index 85852d9d41..19e6dc371d 100644 --- 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() .type() - .should.equal(Portal) + .should.equal(Ref) }) it('renders to the document body', () => { @@ -59,9 +62,7 @@ describe('Popup', () => { it('renders child text', () => { wrapperMount(child text) - 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( - foo} - on='click' - />, + foo} 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(foo} />) + 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( { it('it appears on multiple', (done) => { const trigger = - const button = wrapperMount() - .find('button') + const button = wrapperMount( + , + ).find('button') button.simulate('click') assertInBody('.ui.popup.visible')