From f51c277c1e852695ada58454cded83b23d938cf0 Mon Sep 17 00:00:00 2001 From: Lam Hieu Date: Mon, 20 May 2019 13:15:36 +0700 Subject: [PATCH 1/3] fix(Modal): handle init modal in SSR --- src/Modal.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Modal.js b/src/Modal.js index c698fbc84..6cf398154 100644 --- a/src/Modal.js +++ b/src/Modal.js @@ -90,6 +90,7 @@ class Modal extends React.Component { this._element = null; this._originalBodyPadding = null; + this.isSSR = typeof window === 'function'; this.getFocusableChildren = this.getFocusableChildren.bind(this); this.handleBackdropClick = this.handleBackdropClick.bind(this); this.handleBackdropMouseDown = this.handleBackdropMouseDown.bind(this); @@ -103,12 +104,16 @@ class Modal extends React.Component { isOpen: props.isOpen, }; - if (props.isOpen) { + if (!this.isSSR && props.isOpen) { this.init(); } } componentDidMount() { + if (this.isSSR && this.props.isOpen) { + this.init(); + } + if (this.props.onEnter) { this.props.onEnter(); } From af9d9462bbda150bcac95b1d799fffa771c57842 Mon Sep 17 00:00:00 2001 From: Lam Hieu Date: Tue, 21 May 2019 22:38:14 +0700 Subject: [PATCH 2/3] fix(Modal): move init to did mount of component and rewrite test cases --- src/Modal.js | 9 +--- src/__tests__/Modal.spec.js | 92 ++++++++++++++++++++----------------- 2 files changed, 50 insertions(+), 51 deletions(-) diff --git a/src/Modal.js b/src/Modal.js index 6cf398154..6286a4498 100644 --- a/src/Modal.js +++ b/src/Modal.js @@ -90,7 +90,6 @@ class Modal extends React.Component { this._element = null; this._originalBodyPadding = null; - this.isSSR = typeof window === 'function'; this.getFocusableChildren = this.getFocusableChildren.bind(this); this.handleBackdropClick = this.handleBackdropClick.bind(this); this.handleBackdropMouseDown = this.handleBackdropMouseDown.bind(this); @@ -103,14 +102,10 @@ class Modal extends React.Component { this.state = { isOpen: props.isOpen, }; - - if (!this.isSSR && props.isOpen) { - this.init(); - } } componentDidMount() { - if (this.isSSR && this.props.isOpen) { + if (this.props.isOpen) { this.init(); } @@ -346,7 +341,6 @@ class Modal extends React.Component { } = this.props; if (!!this._element && (this.state.isOpen || !unmountOnClose)) { - const isModalHidden = !!this._element && !this.state.isOpen && !unmountOnClose; this._element.style.display = isModalHidden ? 'none' : 'block'; @@ -420,7 +414,6 @@ class Modal extends React.Component { ); } - return null; } } diff --git a/src/__tests__/Modal.spec.js b/src/__tests__/Modal.spec.js index c1d039084..77b642aae 100644 --- a/src/__tests__/Modal.spec.js +++ b/src/__tests__/Modal.spec.js @@ -3,6 +3,12 @@ import React from 'react'; import { mount, shallow } from 'enzyme'; import { Modal, ModalBody, ModalHeader, ModalFooter, Button } from '../'; +const didMount = (component) => { + const wrapper = mount(component); + wrapper.setProps({ fakefield: 'fakeToUpdate' }); + return wrapper; +} + describe('Modal', () => { let isOpen; let toggle; @@ -28,7 +34,7 @@ describe('Modal', () => { it('should render modal portal into DOM', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -41,7 +47,7 @@ describe('Modal', () => { it('should render with the class "modal-dialog"', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -54,7 +60,7 @@ describe('Modal', () => { it('should render external content when present', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( ×}> Yo! @@ -68,7 +74,7 @@ describe('Modal', () => { it('should render with the backdrop with the class "modal-backdrop" by default', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -81,7 +87,7 @@ describe('Modal', () => { it('should render with the backdrop with the class "modal-backdrop" when backdrop is "static"', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -94,7 +100,7 @@ describe('Modal', () => { it('should not render with the backdrop with the class "modal-backdrop" when backdrop is "false"', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -108,7 +114,7 @@ describe('Modal', () => { it('should render with the class "modal-dialog-scrollable" when scrollable is "true"', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -121,7 +127,7 @@ describe('Modal', () => { it('should render with class "modal-dialog" and have custom class name if provided', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -135,7 +141,7 @@ describe('Modal', () => { it('should render with class "modal-dialog" w/o centered class if not provided', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -149,7 +155,7 @@ describe('Modal', () => { it('should render with class "modal-dialog" and centered class if provided', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -163,7 +169,7 @@ describe('Modal', () => { it('should render with additional props if provided', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -177,7 +183,7 @@ describe('Modal', () => { it('should render without fade transition if provided with fade={false}', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Howdy! @@ -198,7 +204,7 @@ describe('Modal', () => { it('should render when expected when passed modalTransition and backdropTransition props', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( { it('should render with class "modal" and have custom class name if provided with modalClassName', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -234,7 +240,7 @@ describe('Modal', () => { it('should render with custom class name if provided with wrapClassName', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -247,7 +253,7 @@ describe('Modal', () => { it('should render with class "modal-content" and have custom class name if provided with contentClassName', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -260,7 +266,7 @@ describe('Modal', () => { it('should render with class "modal-backdrop" and have custom class name if provided with backdropClassName', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -273,7 +279,7 @@ describe('Modal', () => { it('should render with the class "modal-${size}" when size is passed', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -288,7 +294,7 @@ describe('Modal', () => { it('should render modal when isOpen is true', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -302,7 +308,7 @@ describe('Modal', () => { it('should render modal with default role of "dialog"', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -315,7 +321,7 @@ describe('Modal', () => { it('should render modal with provided role', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -328,7 +334,7 @@ describe('Modal', () => { it('should render modal with aria-labelledby provided labelledBy', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -340,7 +346,7 @@ describe('Modal', () => { }); it('should not render modal when isOpen is false', () => { - const wrapper = mount( + const wrapper = didMount( Yo! @@ -354,7 +360,7 @@ describe('Modal', () => { }); it('should toggle modal', () => { - const wrapper = mount( + const wrapper = didMount( Yo! @@ -383,7 +389,7 @@ describe('Modal', () => { jest.spyOn(Modal.prototype, 'onClosed'); const onOpened = jest.fn(); const onClosed = jest.fn(); - const wrapper = mount( + const wrapper = didMount( Yo! @@ -424,7 +430,7 @@ describe('Modal', () => { it('should call onClosed & onOpened when fade={false}', () => { const onOpened = jest.fn(); const onClosed = jest.fn(); - const wrapper = mount( + const wrapper = didMount( Yo! @@ -485,7 +491,7 @@ describe('Modal', () => { it('should close modal when escape key pressed', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -528,7 +534,7 @@ describe('Modal', () => { it('should not close modal when escape key pressed when keyboard is false', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -563,7 +569,7 @@ describe('Modal', () => { it('should close modal when clicking backdrop', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( @@ -598,7 +604,7 @@ describe('Modal', () => { it('should not close modal when clicking backdrop and backdrop is "static"', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( @@ -624,7 +630,7 @@ describe('Modal', () => { it('should destroy this._element', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( @@ -648,7 +654,7 @@ describe('Modal', () => { it('should destroy this._element when unmountOnClose prop set to true', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( @@ -672,7 +678,7 @@ describe('Modal', () => { it('should not destroy this._element when unmountOnClose prop set to false', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( @@ -696,7 +702,7 @@ describe('Modal', () => { it('should destroy this._element on unmount', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( @@ -715,7 +721,7 @@ describe('Modal', () => { it('should render nested modals', () => { isOpen = true; isOpenNested = true; - const wrapper = mount( + const wrapper = didMount( @@ -744,7 +750,7 @@ describe('Modal', () => { // set a body class which includes modal-open document.body.className = 'my-modal-opened'; - const wrapper = mount( + const wrapper = didMount( Yo! @@ -786,7 +792,7 @@ describe('Modal', () => { it('should call onEnter & onExit props if provided', () => { const onEnter = jest.fn(); const onExit = jest.fn(); - const wrapper = mount( + const wrapper = didMount( Yo! @@ -837,7 +843,7 @@ describe('Modal', () => { it('should allow focus on only focusable elements', () => { isOpen = true; - const wrapper = mount( + const wrapper = didMount( Modal title @@ -876,7 +882,7 @@ describe('Modal', () => { const mock = jest.fn(); - const wrapper = mount( + const wrapper = didMount( @@ -902,7 +908,7 @@ describe('Modal', () => { ); - const wrapper = mount(); + const wrapper = didMount(); const button = wrapper .find('.focus') .hostNodes() @@ -925,7 +931,7 @@ describe('Modal', () => { ); - const wrapper = mount(); + const wrapper = didMount(); const button = wrapper .find('.focus') .hostNodes() @@ -948,7 +954,7 @@ describe('Modal', () => { ); - const wrapper = mount(); + const wrapper = didMount(); const button = wrapper .find('.focus') .hostNodes() @@ -971,7 +977,7 @@ describe('Modal', () => { ); - const wrapper = mount(); + const wrapper = didMount(); const button = wrapper .find('.focus') .hostNodes() From 8a11ea0c6c3fbb1105059dac2938cd9cb32a43c2 Mon Sep 17 00:00:00 2001 From: Lam Hieu Date: Tue, 21 May 2019 23:08:14 +0700 Subject: [PATCH 3/3] fix(Modal): handle tigger to re-render in case `isOpen` is true on init --- src/Modal.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Modal.js b/src/Modal.js index 6286a4498..e663a9dc9 100644 --- a/src/Modal.js +++ b/src/Modal.js @@ -100,13 +100,14 @@ class Modal extends React.Component { this.manageFocusAfterClose = this.manageFocusAfterClose.bind(this); this.state = { - isOpen: props.isOpen, + isOpen: false, }; } componentDidMount() { if (this.props.isOpen) { this.init(); + this.setState({ isOpen: true }) } if (this.props.onEnter) {