From 46eb1bbfebd37ab90d6f3dcc41c254d2c31ce793 Mon Sep 17 00:00:00 2001 From: Kyle Tsang <6854874+kyletsang@users.noreply.github.com> Date: Thu, 15 Jul 2021 09:03:09 -0700 Subject: [PATCH] fix: use shared modal manager to sync container state (#5917) * fix: use shared modal manager to sync container state * fix types * prettier --- src/BootstrapModalManager.tsx | 10 ++++- src/Modal.tsx | 19 ++++------ src/Offcanvas.tsx | 34 ++++++++--------- src/helpers.ts | 6 ++- test/ModalSpec.js | 20 ++++++++++ test/OffcanvasSpec.js | 52 +++++++++++++++++--------- www/src/examples/Offcanvas/Backdrop.js | 19 ++++++---- 7 files changed, 103 insertions(+), 57 deletions(-) diff --git a/src/BootstrapModalManager.tsx b/src/BootstrapModalManager.tsx index 646dc75a5f..edabf1a0a3 100644 --- a/src/BootstrapModalManager.tsx +++ b/src/BootstrapModalManager.tsx @@ -9,7 +9,7 @@ const Selector = { NAVBAR_TOGGLER: '.navbar-toggler', }; -export default class BootstrapModalManager extends ModalManager { +class BootstrapModalManager extends ModalManager { private adjustAndStore( prop: T, element: HTMLElement, @@ -66,3 +66,11 @@ export default class BootstrapModalManager extends ModalManager { ); } } + +let sharedManager: BootstrapModalManager | undefined; +export function getSharedManager() { + if (!sharedManager) sharedManager = new BootstrapModalManager(); + return sharedManager; +} + +export default BootstrapModalManager; diff --git a/src/Modal.tsx b/src/Modal.tsx index dbe2daeee1..2f25df4f5a 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -13,7 +13,8 @@ import PropTypes from 'prop-types'; import * as React from 'react'; import { useCallback, useMemo, useRef, useState } from 'react'; import BaseModal, { BaseModalProps } from 'react-overlays/Modal'; -import BootstrapModalManager from './BootstrapModalManager'; +import { ModalInstance } from 'react-overlays/ModalManager'; +import { getSharedManager } from './BootstrapModalManager'; import Fade from './Fade'; import ModalBody from './ModalBody'; import ModalContext from './ModalContext'; @@ -52,8 +53,6 @@ export interface ModalProps [other: string]: any; } -let manager; - const propTypes = { /** * @default 'modal' @@ -284,9 +283,7 @@ const Modal: BsPrefixRefForwardingComponent<'div', ModalProps> = const waitingForMouseUpRef = useRef(false); const ignoreBackdropClickRef = useRef(false); const removeStaticModalAnimationRef = useRef<(() => void) | null>(null); - - // TODO: what's this type - const [modal, setModalRef] = useCallbackRef<{ dialog: any }>(); + const [modal, setModalRef] = useCallbackRef(); const mergedRef = useMergedRefs(ref, setModalRef); const handleHide = useEventCallback(onHide); @@ -301,15 +298,15 @@ const Modal: BsPrefixRefForwardingComponent<'div', ModalProps> = function getModalManager() { if (propsManager) return propsManager; - if (!manager) manager = new BootstrapModalManager(); - return manager; + return getSharedManager(); } function updateDialogStyle(node) { if (!canUseDOM) return; - const containerIsOverflowing = - getModalManager().isContainerOverflowing(modal); + const containerIsOverflowing = getModalManager().isContainerOverflowing( + modal!, + ); const modalIsOverflowing = node.scrollHeight > ownerDocument(node).documentElement.clientHeight; @@ -358,7 +355,7 @@ const Modal: BsPrefixRefForwardingComponent<'div', ModalProps> = const handleStaticModalAnimation = () => { setAnimateStaticModal(true); removeStaticModalAnimationRef.current = transitionEnd( - modal!.dialog, + modal!.dialog as any, () => { setAnimateStaticModal(false); }, diff --git a/src/Offcanvas.tsx b/src/Offcanvas.tsx index b7b42f7c09..5d4f629011 100644 --- a/src/Offcanvas.tsx +++ b/src/Offcanvas.tsx @@ -1,5 +1,4 @@ import classNames from 'classnames'; -import useCallbackRef from '@restart/hooks/useCallbackRef'; import useEventCallback from '@restart/hooks/useEventCallback'; import PropTypes from 'prop-types'; import * as React from 'react'; @@ -8,8 +7,6 @@ import BaseModal, { ModalProps as BaseModalProps, ModalHandle, } from 'react-overlays/Modal'; -import ModalManager from 'react-overlays/ModalManager'; -import useRootClose from 'react-overlays/useRootClose'; import Fade from './Fade'; import OffcanvasBody from './OffcanvasBody'; import OffcanvasToggling from './OffcanvasToggling'; @@ -18,6 +15,9 @@ import OffcanvasHeader from './OffcanvasHeader'; import OffcanvasTitle from './OffcanvasTitle'; import { BsPrefixRefForwardingComponent } from './helpers'; import { useBootstrapPrefix } from './ThemeProvider'; +import BootstrapModalManager, { + getSharedManager, +} from './BootstrapModalManager'; export type OffcanvasPlacement = 'start' | 'end' | 'top' | 'bottom'; @@ -217,18 +217,12 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> = }, ref, ) => { - const [dialogElement, setDialogElement] = useCallbackRef(); - const modalManager = useRef(); + const modalManager = useRef(); const handleHide = useEventCallback(onHide); bsPrefix = useBootstrapPrefix(bsPrefix, 'offcanvas'); const modalBsPrefix = useBootstrapPrefix(undefined, 'modal'); - // If there's a backdrop, let BaseModal handle closing. - useRootClose(dialogElement, handleHide, { - disabled: backdrop, - }); - const modalContext = useMemo( () => ({ onHide: handleHide, @@ -238,23 +232,27 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> = function getModalManager() { if (propsManager) return propsManager; - if (!modalManager.current) - modalManager.current = new ModalManager({ - handleContainerOverflow: !scroll, - }); - return modalManager.current; + if (scroll) { + // Have to use a different modal manager since the shared + // one handles overflow. + if (!modalManager.current) + modalManager.current = new BootstrapModalManager({ + handleContainerOverflow: false, + }); + return modalManager.current; + } + + return getSharedManager(); } const handleEnter = (node, ...args) => { if (node) node.style.visibility = 'visible'; onEnter?.(node, ...args); - setDialogElement(node); }; const handleExited = (node, ...args) => { if (node) node.style.visibility = ''; onExited?.(...args); - setDialogElement(null); }; const renderBackdrop = useCallback( @@ -295,7 +293,7 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> = container={container} keyboard={keyboard} autoFocus={autoFocus} - enforceFocus={enforceFocus} + enforceFocus={enforceFocus && !scroll} restoreFocus={restoreFocus} restoreFocusOptions={restoreFocusOptions} onEscapeKeyDown={onEscapeKeyDown} diff --git a/src/helpers.ts b/src/helpers.ts index b86685abe8..f472a100f4 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -40,8 +40,10 @@ export class BsPrefixComponent< > extends React.Component & P>> {} // Need to use this instead of typeof Component to get proper type checking. -export type BsPrefixComponentClass = - React.ComponentClass & P>>; +export type BsPrefixComponentClass< + As extends React.ElementType, + P = unknown, +> = React.ComponentClass & P>>; export type SelectCallback = ( eventKey: string | null, diff --git a/test/ModalSpec.js b/test/ModalSpec.js index a1fb798982..83887192c2 100644 --- a/test/ModalSpec.js +++ b/test/ModalSpec.js @@ -1,5 +1,6 @@ import { mount } from 'enzyme'; import * as React from 'react'; +import ModalManager from 'react-overlays/ModalManager'; import Modal from '../src/Modal'; describe('', () => { @@ -407,4 +408,23 @@ describe('', () => { expect(onEscapeKeyDownSpy).to.not.have.been.called; }); + + it('Should use custom props manager if specified', (done) => { + const noOp = () => {}; + + class MyModalManager extends ModalManager { + add() { + done(); + } + } + + const managerRef = React.createRef(); + managerRef.current = new MyModalManager(); + + mount( + + Message + , + ); + }); }); diff --git a/test/OffcanvasSpec.js b/test/OffcanvasSpec.js index 50d00e21a1..e90a60331c 100644 --- a/test/OffcanvasSpec.js +++ b/test/OffcanvasSpec.js @@ -1,6 +1,6 @@ import { mount } from 'enzyme'; import * as React from 'react'; -import simulant from 'simulant'; +// import simulant from 'simulant'; import ModalManager from 'react-overlays/ModalManager'; import Offcanvas from '../src/Offcanvas'; @@ -120,23 +120,25 @@ describe('', () => { expect(onHideSpy).to.have.been.called; }); - it('Should close when anything outside offcanvas clicked and backdrop=false', () => { - const onHideSpy = sinon.spy(); - mount( - <> - - Message - - - , - ); - - simulant.fire(document.body, 'click'); - - expect(onHideSpy).to.have.been.called; - }); + // TODO: unsure if we need this, since it seems like Offcanvas is still undergoing some + // changes upstream. + // it('Should close when anything outside offcanvas clicked and backdrop=false', () => { + // const onHideSpy = sinon.spy(); + // mount( + // <> + // + // Message + // + // + // , + // ); + + // simulant.fire(document.body, 'click'); + + // expect(onHideSpy).to.have.been.called; + // }); it('Should not call onHide if the click target comes from inside the offcanvas', () => { const onHideSpy = sinon.spy(); @@ -227,4 +229,18 @@ describe('', () => { , ); }); + + it('should not change overflow style when scroll=true', () => { + const containerRef = React.createRef(); + const noOp = () => {}; + mount( +
+ + Message + +
, + ); + + expect(containerRef.current.style.overflow).to.equal('scroll'); + }); }); diff --git a/www/src/examples/Offcanvas/Backdrop.js b/www/src/examples/Offcanvas/Backdrop.js index c2695daa69..c72c0e60b8 100644 --- a/www/src/examples/Offcanvas/Backdrop.js +++ b/www/src/examples/Offcanvas/Backdrop.js @@ -1,14 +1,19 @@ const options = [ - { - name: 'Enable body scrolling', - scroll: true, - backdrop: false, - }, { name: 'Enable backdrop (default)', scroll: false, backdrop: true, }, + { + name: 'Disable backdrop', + scroll: false, + backdrop: false, + }, + { + name: 'Enable body scrolling', + scroll: true, + backdrop: false, + }, { name: 'Enable both scrolling & backdrop', scroll: true, @@ -20,11 +25,11 @@ function OffCanvasExample({ name, ...props }) { const [show, setShow] = useState(false); const handleClose = () => setShow(false); - const handleShow = () => setShow(true); + const toggleShow = () => setShow((s) => !s); return ( <> -