From 9bc490522ebb8fe04d338535feaa979dbd9ea168 Mon Sep 17 00:00:00 2001 From: Jay Merrifield Date: Mon, 18 Dec 2017 16:45:29 -0500 Subject: [PATCH 1/3] fix(Popup) popup appearing in wrong position when open prop is set --- src/modules/Popup/Popup.js | 49 ++++++++++++++++---------- test/specs/modules/Popup/Popup-test.js | 18 +++++++++- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/modules/Popup/Popup.js b/src/modules/Popup/Popup.js index c62cd14acf..decb323bcd 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') @@ -157,27 +158,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 @@ -231,7 +233,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) @@ -323,6 +325,13 @@ export default class Popup extends Component { this.setPopupStyle() } + handleTriggerRef = (triggerRef) => { + debug('triggerMounted()') + if (triggerRef) { + this.triggerRef = triggerRef + } + } + render() { const { basic, @@ -376,16 +385,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 41f0950ae5..4fd1b5d55e 100644 --- a/test/specs/modules/Popup/Popup-test.js +++ b/test/specs/modules/Popup/Popup-test.js @@ -47,6 +47,7 @@ describe('Popup', () => { it('renders a Portal', () => { wrapperShallow() + .dive() .type() .should.equal(Portal) }) @@ -110,7 +111,7 @@ describe('Popup', () => { describe('position', () => { POSITIONS.forEach((position) => { - it('is always within the viewport', () => { + it('is always within the viewport when trigger is clicked', () => { wrapperMount( { 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 position fits within the viewport', () => { wrapperMount( Date: Tue, 8 May 2018 10:46:36 -0700 Subject: [PATCH 2/3] fix(Popup): apply RAF to pin Popup to trigger component --- src/modules/Popup/Popup.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/modules/Popup/Popup.js b/src/modules/Popup/Popup.js index decb323bcd..0662f76ffe 100644 --- a/src/modules/Popup/Popup.js +++ b/src/modules/Popup/Popup.js @@ -308,6 +308,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) } @@ -332,6 +333,14 @@ export default class Popup extends Component { } } + setPosition = () => { + if (this.triggerRef) { + this.setPopupStyle(this.props.position) + } + + this.animationRequestId = requestAnimationFrame(this.setPosition) + } + render() { const { basic, From 7c2104fb83945dbe4544a58b481df1eefb9fdcdc Mon Sep 17 00:00:00 2001 From: cdaringe Date: Tue, 8 May 2018 13:01:31 -0700 Subject: [PATCH 3/3] fix(Popup): cancel animation on unMount --- src/modules/Popup/Popup.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/modules/Popup/Popup.js b/src/modules/Popup/Popup.js index 8628a77237..e7900addf8 100644 --- a/src/modules/Popup/Popup.js +++ b/src/modules/Popup/Popup.js @@ -334,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) }