fix: handle onScrollLock cleanly, dialog to render inside parent elem…

…ent (#168)
EightfoldAI · Jun 6, 2022 · e9a5f0e · e9a5f0e
1 parent 25f187c
commit e9a5f0e
Show file tree

Hide file tree

Showing 11 changed files with 103 additions and 43 deletions.
diff --git a/src/__snapshots__/storybook.test.js.snap b/src/__snapshots__/storybook.test.js.snap
@@ -5774,6 +5774,22 @@ exports[`Storyshots Modal Small 1`] = `
 </button>
 `;
 
+exports[`Storyshots Modal X Large 1`] = `
+<button
+  aria-disabled={false}
+  className="button buttonPrimary"
+  defaultChecked={false}
+  disabled={false}
+  onClick={[Function]}
+>
+  <span
+    className=""
+  >
+    Open modal
+  </span>
+</button>
+`;
+
 exports[`Storyshots Pagination All Combined 1`] = `
 <div
   className="my-pagination-class pagination"
@@ -5786,7 +5802,7 @@ exports[`Storyshots Pagination All Combined 1`] = `
       className="mainWrapper"
     >
       <button
-        aria-controls="dropdown-14"
+        aria-controls="dropdown-15"
         aria-disabled={false}
         aria-expanded={false}
         aria-haspopup={true}
@@ -5972,7 +5988,7 @@ exports[`Storyshots Pagination All Combined 1`] = `
         className="editor"
         defaultValue={4}
         disabled={false}
-        id="input-15"
+        id="input-16"
         minLength={1}
         onChange={[Function]}
         required={false}
@@ -6301,7 +6317,7 @@ exports[`Storyshots Pagination Change Page Size 1`] = `
       className="mainWrapper"
     >
       <button
-        aria-controls="dropdown-16"
+        aria-controls="dropdown-17"
         aria-disabled={false}
         aria-expanded={false}
         aria-haspopup={true}
@@ -6747,7 +6763,7 @@ exports[`Storyshots Pagination Jump To 1`] = `
         className="editor"
         defaultValue={5}
         disabled={false}
-        id="input-17"
+        id="input-18"
         minLength={1}
         onChange={[Function]}
         required={false}

diff --git a/src/components/Dialog/BaseDialog/BaseDialog.tsx b/src/components/Dialog/BaseDialog/BaseDialog.tsx
@@ -31,13 +31,15 @@ export const BaseDialog: FC<BaseDialogProps> = React.forwardRef(
             actionsClassNames,
             dialogWrapperClassNames,
             dialogClassNames,
+            positionStrategy = 'absolute',
+            style,
             ...rest
         },
         ref: Ref<HTMLDivElement>
     ) => {
         const labelId = uniqueId('dialog-label-');
 
-        const { lockScroll, unlockScroll } = useScrollLock(parent);
+        useScrollLock(parent, visible);
 
         const dialogBackdropClasses: string = mergeClasses([
             styles.dialogBackdrop,
@@ -55,6 +57,11 @@ export const BaseDialog: FC<BaseDialogProps> = React.forwardRef(
             headerClassNames,
         ]);
 
+        const dialogBackdropStyle: React.CSSProperties = {
+            position: positionStrategy,
+            ...style,
+        };
+
         const dialogStyle: React.CSSProperties = {
             zIndex,
             height,
@@ -63,11 +70,6 @@ export const BaseDialog: FC<BaseDialogProps> = React.forwardRef(
 
         useEffect(() => {
             onVisibleChange?.(visible);
-            if (visible) {
-                lockScroll();
-            } else {
-                unlockScroll();
-            }
         }, [visible]);
 
         const getDialog = (): JSX.Element => (
@@ -77,6 +79,7 @@ export const BaseDialog: FC<BaseDialogProps> = React.forwardRef(
                 role="dialog"
                 aria-modal={true}
                 aria-labelledby={labelId}
+                style={dialogBackdropStyle}
                 className={dialogBackdropClasses}
                 onClick={(e: React.MouseEvent<HTMLDivElement>) => {
                     maskClosable && onClose?.(e);

diff --git a/src/components/Dialog/BaseDialog/BaseDialog.types.ts b/src/components/Dialog/BaseDialog/BaseDialog.types.ts
@@ -5,6 +5,8 @@ type EventType =
     | React.KeyboardEvent<HTMLDivElement>
     | React.MouseEvent<HTMLDivElement | HTMLButtonElement>;
 
+type Strategy = 'absolute' | 'fixed';
+
 export interface BaseDialogProps
     extends Omit<OcBaseProps<HTMLDivElement>, 'classNames'> {
     /**
@@ -70,6 +72,11 @@ export interface BaseDialogProps
      * Custom zIndex for the dialog
      */
     zIndex?: number;
+    /**
+     * Positioning strategy for the dialog
+     * @default absolute
+     */
+    positionStrategy?: Strategy;
     /**
      * Element to which to attach the modal to
      * @default HTMLBodyElement

diff --git a/src/components/Dialog/Dialog.stories.tsx b/src/components/Dialog/Dialog.stories.tsx
@@ -144,6 +144,10 @@ export default {
             options: [DialogSize.medium, DialogSize.small],
             control: { type: 'inline-radio' },
         },
+        positionStrategy: {
+            options: ['absolute', 'fixed'],
+            control: { type: 'inline-radio' },
+        },
     },
 } as ComponentMeta<typeof Dialog>;
 
@@ -208,7 +212,7 @@ const dialogArgs: Object = {
     dialogWrapperClassNames: 'my-dialog-wrapper-class',
     header: 'Header 4 used in this dialog',
     headerClassNames: 'my-dialog-header-class',
-    maskClosable: false,
+    maskClosable: true,
     okButtonProps: {
         ariaLabel: 'OK',
         classNames: 'my-ok-btn-class',
@@ -219,6 +223,7 @@ const dialogArgs: Object = {
     },
     parent: document.body,
     zIndex: 1000,
+    positionStrategy: 'absolute',
 };
 
 Medium.args = {

diff --git a/src/components/Modal/Modal.stories.tsx b/src/components/Modal/Modal.stories.tsx
@@ -81,6 +81,10 @@ export default {
             ],
             control: { type: 'select' },
         },
+        positionStrategy: {
+            options: ['absolute', 'fixed'],
+            control: { type: 'inline-radio' },
+        },
     },
 } as ComponentMeta<typeof Modal>;
 
@@ -141,6 +145,25 @@ const Large_Story: ComponentStory<typeof Modal> = (args) => {
 
 export const Large = Large_Story.bind({});
 
+const XLarge_Story: ComponentStory<typeof Modal> = (args) => {
+    const [visible, setVisible] = useState<boolean>(false);
+    return (
+        <>
+            <PrimaryButton
+                text={'Open modal'}
+                onClick={() => setVisible(true)}
+            />
+            <Modal
+                {...args}
+                onClose={() => setVisible(false)}
+                visible={visible}
+            />
+        </>
+    );
+};
+
+export const XLarge = XLarge_Story.bind({});
+
 const Fullscreen_Story: ComponentStory<typeof Modal> = (args) => {
     const [visible, setVisible] = useState<boolean>(false);
     return (
@@ -203,6 +226,8 @@ const modalArgs: Object = {
     modalWrapperClassNames: 'my-modal-wrapper-class',
     parent: document.body,
     zIndex: 1000,
+    maskClosable: false,
+    positionStrategy: 'absolute',
 };
 
 Small.args = {
@@ -219,6 +244,11 @@ Large.args = {
     size: ModalSize.large,
 };
 
+XLarge.args = {
+    ...modalArgs,
+    size: ModalSize.xLarge,
+};
+
 Fullscreen.args = {
     ...modalArgs,
     size: ModalSize.fullscreen,

diff --git a/src/components/Modal/Modal.tsx b/src/components/Modal/Modal.tsx
@@ -24,6 +24,7 @@ export const Modal: FC<ModalProps> = React.forwardRef(
             { [styles.small]: size === ModalSize.small },
             { [styles.medium]: size === ModalSize.medium },
             { [styles.large]: size === ModalSize.large },
+            { [styles.xLarge]: size === ModalSize.xLarge },
             { [styles.fullscreen]: size === ModalSize.fullscreen },
         ]);
 

diff --git a/src/components/Modal/Modal.types.ts b/src/components/Modal/Modal.types.ts
@@ -4,6 +4,7 @@ export enum ModalSize {
     small = 'small',
     medium = 'medium',
     large = 'large',
+    xLarge = 'xlarge',
     fullscreen = 'fullscreen',
 }
 

diff --git a/src/components/Modal/modal.module.scss b/src/components/Modal/modal.module.scss
@@ -30,6 +30,14 @@
         }
     }
 
+    &.x-large {
+        width: min(85vw, 1500px);
+
+        @media (max-width: $small-screen-size) {
+            width: 95vw;
+        }
+    }
+
     &.fullscreen {
         width: 100%;
         height: 100%;

diff --git a/src/components/Panel/Panel.tsx b/src/components/Panel/Panel.tsx
@@ -57,7 +57,7 @@ export const Panel = React.forwardRef<PanelRef, PanelProps>(
         const containerRef = useRef<HTMLDivElement>(null);
         const parentPanel = useContext<PanelRef>(PanelContext);
         const [internalPush, setPush] = useState<boolean>(false);
-        const { lockScroll, unlockScroll } = useScrollLock(parent);
+        useScrollLock(parent, visible);
 
         const panelBackdropClasses: string = mergeClasses([
             styles.panelBackdrop,
@@ -96,12 +96,6 @@ export const Panel = React.forwardRef<PanelRef, PanelProps>(
                 } else {
                     parentPanel.pull();
                 }
-            } else {
-                if (visible) {
-                    lockScroll();
-                } else {
-                    unlockScroll();
-                }
             }
             if (autoFocus) {
                 setTimeout(() => {

diff --git a/src/hooks/useScrollLock.test.tsx b/src/hooks/useScrollLock.test.tsx
@@ -1,24 +1,16 @@
-import { act, renderHook } from '@testing-library/react-hooks';
+import { renderHook } from '@testing-library/react-hooks';
 
 import { useScrollLock } from './useScrollLock';
+import { useState } from 'react';
 
 describe('useScrollLock hook', () => {
     it('set overflow to hidden and reset', () => {
-        const { result } = renderHook(() =>
-            useScrollLock(document.documentElement)
-        );
-        const { unlockScroll, lockScroll } = result.current;
-
-        act(() => {
-            lockScroll();
+        renderHook(() => {
+            const [visible, setVisible] = useState<boolean>(false);
+            useScrollLock(document.documentElement, visible);
+            expect(document.documentElement.style.overflow).toBe('hidden');
+            setVisible(false);
+            expect(document.documentElement.style.overflow).toBe('');
         });
-
-        expect(document.documentElement.style.overflow).toBe('hidden');
-
-        act(() => {
-            unlockScroll();
-        });
-
-        expect(document.documentElement.style.overflow).toBe('');
     });
 });
diff --git a/src/hooks/useScrollLock.ts b/src/hooks/useScrollLock.ts
@@ -1,23 +1,26 @@
-import React from 'react';
+import React, { useEffect, useRef } from 'react';
 
 /**
  * Hook to lock scroll on the given element
  * @param element
+ * @param shouldLock
  */
-export const useScrollLock = (element: HTMLElement) => {
-    let originalOverflow: string;
+export const useScrollLock = (element: HTMLElement, shouldLock: boolean) => {
+    let originalOverflow = useRef<string>('');
 
     const lockScroll = React.useCallback(() => {
-        originalOverflow = element.style.overflow;
+        originalOverflow.current = element.style.overflow;
         element.style.overflow = 'hidden';
     }, []);
 
     const unlockScroll = React.useCallback(() => {
-        element.style.overflow = originalOverflow;
+        element.style.overflow = originalOverflow.current;
     }, []);
 
-    return {
-        lockScroll,
-        unlockScroll,
-    };
+    useEffect(() => {
+        if (shouldLock) {
+            lockScroll();
+        }
+        return unlockScroll;
+    }, [element, shouldLock]);
 };