Browse Source

feat(modal) Consolidate close prevention props (#43258)

Currently, there are two props that do the same thing: `backdrop:
'static'` and `allowClickClose: false`.

This combines them into `closeEvents: 'all' | 'none' | 'backdrop-click'
| 'escape-key'`

Fewer options, and also adds the ability to disable closing by the
escape key, which was required in a feature that @roggenkemper is
developing.

Note: `backdrop: 'static'` is still around due to uses in getsentry.
Will remove once this and
https://github.com/getsentry/getsentry/pull/9273 are merged.
Malachi Willey 2 years ago
parent
commit
d261d827ee

+ 17 - 5
static/app/actionCreators/modal.tsx

@@ -140,7 +140,10 @@ export async function openEditOwnershipRules(options: EditOwnershipRulesModalOpt
   const mod = await import('sentry/components/modals/editOwnershipRulesModal');
   const {default: Modal, modalCss} = mod;
 
-  openModal(deps => <Modal {...deps} {...options} />, {backdrop: 'static', modalCss});
+  openModal(deps => <Modal {...deps} {...options} />, {
+    closeEvents: 'escape-key',
+    modalCss,
+  });
 }
 
 export async function openCommandPalette(options: ModalOptions = {}) {
@@ -240,14 +243,20 @@ export async function openWidgetBuilderOverwriteModal(
   const mod = await import('sentry/components/modals/widgetBuilder/overwriteWidgetModal');
   const {default: Modal, modalCss} = mod;
 
-  openModal(deps => <Modal {...deps} {...options} />, {backdrop: 'static', modalCss});
+  openModal(deps => <Modal {...deps} {...options} />, {
+    closeEvents: 'escape-key',
+    modalCss,
+  });
 }
 
 export async function openAddToDashboardModal(options) {
   const mod = await import('sentry/components/modals/widgetBuilder/addToDashboardModal');
   const {default: Modal, modalCss} = mod;
 
-  openModal(deps => <Modal {...deps} {...options} />, {backdrop: 'static', modalCss});
+  openModal(deps => <Modal {...deps} {...options} />, {
+    closeEvents: 'escape-key',
+    modalCss,
+  });
 }
 
 export async function openReprocessEventModal({
@@ -286,7 +295,10 @@ export async function openDashboardWidgetQuerySelectorModal(
   const mod = await import('sentry/components/modals/dashboardWidgetQuerySelectorModal');
   const {default: Modal, modalCss} = mod;
 
-  openModal(deps => <Modal {...deps} {...options} />, {backdrop: 'static', modalCss});
+  openModal(deps => <Modal {...deps} {...options} />, {
+    closeEvents: 'escape-key',
+    modalCss,
+  });
 }
 
 export async function openWidgetViewerModal({
@@ -297,7 +309,7 @@ export async function openWidgetViewerModal({
   const {default: Modal, modalCss} = mod;
 
   openModal(deps => <Modal {...deps} {...options} />, {
-    backdrop: 'static',
+    closeEvents: 'escape-key',
     modalCss,
     onClose,
   });

+ 114 - 7
static/app/components/globalModal/index.spec.jsx

@@ -29,7 +29,7 @@ describe('GlobalModal', function () {
     expect(screen.queryByTestId('modal-test')).not.toBeInTheDocument();
   });
 
-  it('calls onClose handler when modal is clicked out of', function () {
+  it('calls onClose handler when close button is clicked', function () {
     renderGlobalModal();
     const closeSpy = jest.fn();
 
@@ -49,6 +49,26 @@ describe('GlobalModal', function () {
     expect(closeSpy).toHaveBeenCalled();
   });
 
+  it('calls onClose handler when modal is clicked out of', function () {
+    renderGlobalModal();
+    const closeSpy = jest.fn();
+
+    act(() =>
+      openModal(
+        ({Header}) => (
+          <div id="modal-test">
+            <Header closeButton>Header</Header>Hi
+          </div>
+        ),
+        {onClose: closeSpy}
+      )
+    );
+
+    userEvent.click(screen.getByTestId('modal-backdrop'));
+
+    expect(closeSpy).toHaveBeenCalled();
+  });
+
   it('calls onClose handler when escape key is pressed', function () {
     renderGlobalModal();
     const closeSpy = jest.fn();
@@ -85,8 +105,8 @@ describe('GlobalModal', function () {
     expect(closeSpy).toHaveBeenCalled();
   });
 
-  it('calls ignores click out when the allowClickClose option is false', async function () {
-    renderGlobalModal();
+  it("ignores click out with backdrop: 'static'", async function () {
+    const {waitForModalToHide} = renderGlobalModal();
     render(<div data-test-id="outside-test">Hello</div>);
 
     act(() =>
@@ -96,19 +116,106 @@ describe('GlobalModal', function () {
             <Header closeButton>Header</Header>Hi
           </div>
         ),
-        {allowClickClose: false}
+        {backdrop: 'static'}
       )
     );
 
     expect(screen.getByTestId('modal-test')).toBeInTheDocument();
 
+    // Clicking outside of modal doesn't close
     userEvent.click(screen.getByTestId('outside-test'));
+    expect(screen.getByTestId('modal-test')).toBeInTheDocument();
 
+    // Pressing escape _does_ close
+    userEvent.keyboard('{Escape}');
     expect(screen.getByTestId('modal-test')).toBeInTheDocument();
+    await waitForModalToHide();
+  });
 
-    userEvent.click(screen.getByRole('button', {name: 'Close Modal'}));
+  it("ignores click out with closeEvents: 'escape-key'", async function () {
+    const {waitForModalToHide} = renderGlobalModal();
+    const closeSpy = jest.fn();
 
-    await waitForElementToBeRemoved(screen.queryByTestId('modal-test'));
-    expect(screen.queryByTestId('modal-test')).not.toBeInTheDocument();
+    act(() =>
+      openModal(
+        ({Header}) => (
+          <div data-test-id="modal-test">
+            <Header closeButton>Header</Header>Hi
+          </div>
+        ),
+        {closeEvents: 'escape-key', onClose: closeSpy}
+      )
+    );
+
+    expect(screen.getByTestId('modal-test')).toBeInTheDocument();
+
+    // Clicking outside of modal doesn't close
+    userEvent.click(screen.getByTestId('modal-backdrop'));
+    expect(screen.getByTestId('modal-test')).toBeInTheDocument();
+    expect(closeSpy).not.toHaveBeenCalled();
+
+    // Pressing escape _does_ close
+    userEvent.keyboard('{Escape}');
+    await waitForModalToHide();
+    expect(closeSpy).toHaveBeenCalled();
+  });
+
+  it("ignores escape key with closeEvents: 'backdrop-click'", async function () {
+    const {waitForModalToHide} = renderGlobalModal();
+    const closeSpy = jest.fn();
+
+    act(() =>
+      openModal(
+        ({Header}) => (
+          <div data-test-id="modal-test">
+            <Header closeButton>Header</Header>Hi
+          </div>
+        ),
+        {closeEvents: 'backdrop-click', onClose: closeSpy}
+      )
+    );
+
+    expect(screen.getByTestId('modal-test')).toBeInTheDocument();
+
+    // Pressing escape doesn't close
+    userEvent.keyboard('{Escape}');
+    expect(screen.getByTestId('modal-test')).toBeInTheDocument();
+    expect(closeSpy).not.toHaveBeenCalled();
+
+    // Clicking outside of modal _does_ close
+    userEvent.click(screen.getByTestId('modal-backdrop'));
+    expect(closeSpy).toHaveBeenCalled();
+    await waitForModalToHide();
+  });
+
+  it("ignores backdrop click and escape key when with closeEvents: 'none'", async function () {
+    const {waitForModalToHide} = renderGlobalModal();
+    const closeSpy = jest.fn();
+
+    act(() =>
+      openModal(
+        ({Header}) => (
+          <div data-test-id="modal-test">
+            <Header closeButton>Header</Header>Hi
+          </div>
+        ),
+        {closeEvents: 'none', onClose: closeSpy}
+      )
+    );
+
+    expect(screen.getByTestId('modal-test')).toBeInTheDocument();
+
+    // Clicking outside of modal doesn't close
+    userEvent.click(screen.getByTestId('modal-backdrop'));
+    expect(closeSpy).not.toHaveBeenCalled();
+
+    // Pressing escape doesn't close
+    userEvent.keyboard('{Escape}');
+    expect(closeSpy).not.toHaveBeenCalled();
+
+    // Clicking an explicit close button does close
+    userEvent.click(screen.getByRole('button', {name: 'Close Modal'}));
+    expect(closeSpy).toHaveBeenCalled();
+    await waitForModalToHide();
   });
 });

+ 39 - 14
static/app/components/globalModal/index.tsx

@@ -18,17 +18,28 @@ import {makeClosableHeader, makeCloseButton, ModalBody, ModalFooter} from './com
 
 type ModalOptions = {
   /**
-   * Set to `false` to disable the ability to click outside the modal to
-   * close it. This is useful for modals containing user input which will
-   * disappear on an accidental click. Defaults to `true`.
+   * Set to `false` to disable the backdrop from being rendered.
+   * Set to `true` (the default) to show a translucent backdrop.
    */
-  allowClickClose?: boolean;
+  backdrop?: 'static' | boolean; // TODO(malwilley): Remove 'static' when no longer used in getsentry
   /**
-   * Set to `false` to disable the backdrop from being rendered. Set to
-   * `static` to disable the 'click outside' behavior from closing the modal.
-   * Set to true (the default) to show a translucent backdrop
+   * By default, the modal is closed when the backdrop is clicked or the
+   * escape key is pressed. This prop allows you to modify that behavior.
+   * Only use when completely necessary, the defaults are important for
+   * accessibility.
+   *
+   * 'all' (default) - the modal is automatically closed on backdrop click or
+   *   escape key.
+   * 'none' - the modal cannot be dismissed with either the mouse or the
+   *   keyboard. The modal will need to be closed manually with `closeModal()`.
+   *   This should only be used when a modal requires user input and cannot be
+   *   dismissed, which is rare.
+   * 'backdrop-click' - the modal cannot be dimissed by pressing the escape key.
+   * 'escape-key' - the modal cannot be dismissed by clicking on the backdrop.
+   *   This is useful for modals containing user input which will disappear on an
+   *   accidental click.
    */
-  backdrop?: 'static' | boolean;
+  closeEvents?: 'all' | 'none' | 'backdrop-click' | 'escape-key';
   /**
    * Additional CSS which will be applied to the modals `role="dialog"`
    * component. You may use the `[role="document"]` selector to target the
@@ -90,6 +101,8 @@ type Props = {
 function GlobalModal({onClose}: Props) {
   const {renderer, options} = useLegacyStore(ModalStore);
 
+  const closeEvents = options.closeEvents ?? 'all';
+
   const visible = typeof renderer === 'function';
 
   const closeModal = useCallback(() => {
@@ -104,8 +117,18 @@ function GlobalModal({onClose}: Props) {
   }, [options, onClose]);
 
   const handleEscapeClose = useCallback(
-    (e: KeyboardEvent) => e.key === 'Escape' && closeModal(),
-    [closeModal]
+    (e: KeyboardEvent) => {
+      if (
+        e.key !== 'Escape' ||
+        closeEvents === 'none' ||
+        closeEvents === 'backdrop-click'
+      ) {
+        return;
+      }
+
+      closeModal();
+    },
+    [closeModal, closeEvents]
   );
 
   const portal = getModalPortal();
@@ -163,13 +186,14 @@ function GlobalModal({onClose}: Props) {
   // Default to enabled backdrop
   const backdrop = options.backdrop ?? true;
 
-  // Default to enabled click close
-  const allowClickClose = options.allowClickClose ?? true;
+  const allowBackdropClickClose =
+    (closeEvents === 'all' || closeEvents === 'backdrop-click') &&
+    options.backdrop !== 'static';
 
   // Only close when we directly click outside of the modal.
   const containerRef = useRef<HTMLDivElement>(null);
   const clickClose = (e: React.MouseEvent) =>
-    containerRef.current === e.target && allowClickClose && closeModal();
+    containerRef.current === e.target && allowBackdropClickClose && closeModal();
 
   return createPortal(
     <Fragment>
@@ -177,9 +201,10 @@ function GlobalModal({onClose}: Props) {
         style={backdrop && visible ? {opacity: 0.5, pointerEvents: 'auto'} : {}}
       />
       <Container
+        data-test-id="modal-backdrop"
         ref={containerRef}
         style={{pointerEvents: visible ? 'auto' : 'none'}}
-        onClick={backdrop === true ? clickClose : undefined}
+        onClick={backdrop ? clickClose : undefined}
       >
         <AnimatePresence>
           {visible && (

+ 1 - 1
static/app/components/group/externalIssueActions.tsx

@@ -65,7 +65,7 @@ const ExternalIssueActions = ({configurations, group, onChange}: Props) => {
   const doOpenModal = (integration: GroupIntegration) =>
     openModal(
       deps => <ExternalIssueForm {...deps} {...{group, onChange, integration}} />,
-      {allowClickClose: false}
+      {closeEvents: 'escape-key'}
     );
 
   return (

+ 1 - 1
static/app/components/group/sentryAppExternalIssueActions.tsx

@@ -79,7 +79,7 @@ class SentryAppExternalIssueActions extends Component<Props, State> {
           onSubmitSuccess={this.onSubmitSuccess}
         />
       ),
-      {allowClickClose: false}
+      {closeEvents: 'escape-key'}
     );
   };
 

+ 1 - 1
static/app/views/alerts/rules/issue/ruleNode.tsx

@@ -568,7 +568,7 @@ function RuleNode({
                       resetValues={data}
                     />
                   ),
-                  {allowClickClose: false}
+                  {closeEvents: 'escape-key'}
                 );
               }}
             >

+ 1 - 1
static/app/views/alerts/rules/metric/triggers/actionsPanel/index.tsx

@@ -388,7 +388,7 @@ class ActionsPanel extends PureComponent<Props> {
                                 }
                               />
                             ),
-                            {allowClickClose: false}
+                            {closeEvents: 'escape-key'}
                           );
                         }}
                       >

+ 1 - 1
static/app/views/eventsV2/table/tableView.tsx

@@ -425,7 +425,7 @@ function TableView(props: TableViewProps) {
           customMeasurements={customMeasurements}
         />
       ),
-      {modalCss, backdrop: 'static'}
+      {modalCss, closeEvents: 'escape-key'}
     );
   }
 

+ 1 - 1
static/app/views/organizationIntegrations/pluginDetailedView.tsx

@@ -120,7 +120,7 @@ class PluginDetailedView extends AbstractIntegrationDetailedView<
           }}
         />
       ),
-      {allowClickClose: false}
+      {closeEvents: 'escape-key'}
     );
   };
 

+ 1 - 1
static/app/views/performance/table.tsx

@@ -137,7 +137,7 @@ class _Table extends Component<Props, State> {
               }}
             />
           ),
-          {modalCss, backdrop: 'static'}
+          {modalCss, closeEvents: 'escape-key'}
         );
         return;
       }

Some files were not shown because too many files changed in this diff