Add close through a child function to dialogs #234

snowystinger · 2020-02-28T22:21:53Z

Closes https://jira.corp.adobe.com/browse/RSP-1533 and https://jira.corp.adobe.com/browse/RSP-1548. Additionally changes the logic for isDismissable modal dialogs so that they can be closed by clicking outside the dialog. Non-dismissable modal dialogs cannot be closed by clicking outside the dialog. Popovers and tray should still always be closable via outside click.

✅ Pull Request Checklist:

Included link to corresponding Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test that the dialog/dialogTrigger stories are all closable via escape, cancel/confirm buttons. Confirm that isDismissable modal dialogs, popover, and tray dialogs can be closed by clicking outside of the dialog. Confirm that normal modal dialogs cannot.

🧢 Your Team:

…en used with a fresh install

move isDismissable to by default be on the context

codecov · 2020-02-28T23:36:40Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9c47417). Click here to learn what that means.
The diff coverage is 50%.

@@           Coverage Diff            @@
##             master    #234   +/-   ##
========================================
  Coverage          ?   71.2%           
========================================
  Files             ?     206           
  Lines             ?    3966           
  Branches          ?     844           
========================================
  Hits              ?    2824           
  Misses            ?     570           
  Partials          ?     572

Impacted Files	Coverage Δ
packages/@react-spectrum/dialog/src/Dialog.tsx	`86.66% <ø> (ø)`
packages/@react-spectrum/dialog/src/context.ts	`100% <ø> (ø)`
...ackages/@react-spectrum/dialog/src/AlertDialog.tsx	`66.66% <0%> (ø)`
...kages/@react-spectrum/dialog/src/DialogTrigger.tsx	`82.75% <66.66%> (ø)`

adobe-bot · 2020-02-29T00:15:22Z

Build successful! 🎉

adobe-bot · 2020-02-29T00:40:53Z

Build successful! 🎉

adobe-bot · 2020-02-29T01:13:45Z

Build successful! 🎉

ktabors · 2020-03-02T19:08:34Z

packages/@react-spectrum/dialog/src/DialogTrigger.tsx

@@ -66,13 +71,13 @@ export function DialogTrigger(props: SpectrumDialogTriggerProps) {
      case 'modal':
        return (
          <Modal isOpen={isOpen} onClose={onClose}>
-            {content}
+            {typeof content === 'function' ? content(onClose) : content}


I'm not a fan of asking the people who create modals to write (onClose) => {<content><Button onPress={close}>Close</Button></content>}. It feels like we're asking them to write jsx that isn't just elements.

The first idea I thought of was a modal context to pass the onClose to button children which is also problematic, but it doesn't make the end user jump through any weird hoops.

I'm concerned that this will be a support issue of us having to train everyone to write things this way.

This is a pretty common React pattern called 'Function as children' so it shouldn't be hard to understand.
This should also be documentable via our prop tables.
There isn't really a great way to do this via context without creating <DialogButton or something equivalent that can pick up a different context than the slots and knows which of the three (four) possible buttons it is.

We went through many options before arriving at this one. I think Devon might have it written down.

ktabors · 2020-03-02T19:18:09Z

packages/@react-spectrum/dialog/test/AlertDialog.test.js

+    expect(onCancelSpy).toHaveBeenCalledWith();
+  });
+
+  it('renders alert dialog with onConfirm / onCancel', function () {


this has the same test description as the previous test, renders alert dialog with onConfirm / onCancel.

Also, why aren't we checking what onConfirmSpy was called with in the previous test and onCancelSpy in this test so we do see that things are different?

packages/@react-spectrum/dialog/test/AlertDialog.test.js

adobe-bot · 2020-03-03T18:51:03Z

Build successful! 🎉

LFDanLu

Just a single question, otherwise just waiting for the removal of the dupe tests that Kyle noted

packages/@react-spectrum/dialog/stories/Dialog.stories.tsx

…rum-v3 into function-as-a-child-dialog-triggers

adobe-bot · 2020-03-05T18:07:47Z

Build successful! 🎉

adobe-bot · 2020-03-05T18:13:10Z

Build successful! 🎉

…rum-v3 into function-as-a-child-dialog-triggers

Popover, tray, tooltip, etc should all still be dismissible by outside interaction click

adobe-bot · 2020-03-06T00:13:13Z

Build successful! 🎉

…rum-v3 into function-as-a-child-dialog-triggers

adobe-bot · 2020-03-06T01:13:34Z

Build successful! 🎉

ktabors · 2020-03-07T04:07:36Z

packages/@react-spectrum/dialog/test/DialogTrigger.test.js

+    function Test({defaultOpen, onOpenChange}) {
+      return (
+        <Provider theme={theme}>
+          <DialogTrigger type="popover" defaultOpen={defaultOpen} onOpenChange={onOpenChange}>


Shouldn't this example have isDismissable on it?

Oh ya lemme add isDismissable=false here

adobe-bot · 2020-03-09T16:36:19Z

Build successful! 🎉

snowystinger · 2020-03-09T19:56:55Z

packages/@react-spectrum/dialog/src/DialogTrigger.tsx

          </Modal>
        );
      case 'tray':
        return (
          <Tray isOpen={isOpen} onClose={onClose}>
-            {content}
+            {typeof content === 'function' ? content(onClose) : content}


hmm... thought tray's couldn't have the buttons? I'm fine to just have it all done the same though, seems useful

adobe-bot · 2020-03-09T20:06:21Z

Build successful! 🎉

ktabors

lgtm

devongovett

Can we handle this automatically for AlertDialog via context so you don't need to wrap it in a close function yourself?

devongovett · 2020-03-09T21:52:11Z

packages/@react-types/dialog/src/index.d.ts

@@ -27,6 +29,18 @@ export interface SpectrumDialogTriggerProps extends PositionProps {
  isDismissable?: boolean
 }

+export interface SpectrumDialogTriggerBase {


Move this into react-spectrum. It isn't a public component.

devongovett · 2020-03-09T21:53:09Z

packages/@react-aria/overlays/src/useOverlay.ts

-  isOpen?: boolean
+  isOpen?: boolean,
+  // Whether to close overlay if underlay is clicked
+  closeOnInteractOutside?: boolean


Can we call this isDismissible and default it to false for consistency?

adobe-bot · 2020-03-09T23:18:16Z

Build successful! 🎉

adobe-bot · 2020-03-09T23:20:59Z

Build successful! 🎉

snowystinger added 2 commits February 28, 2020 11:16

starting work on closable dialogs

c02b5fc

Works, now for types and dismiss button

e574357

snowystinger requested review from dannify and devongovett as code owners February 28, 2020 22:21

snowystinger added 6 commits February 28, 2020 14:22

Merge branch 'master' into function-as-a-child-dialog-triggers

4f0b9a2

update to yarn lock because apparently it didn't resolve correctly wh…

c2fb8ed

…en used with a fresh install

update a bunch of stories and add a test

70fa9ba

Merge branch 'master' into function-as-a-child-dialog-triggers

9af5066

Hook up dismissable

36216d6

appease typescript

ccd6901

move isDismissable to by default be on the context

snowystinger added 4 commits February 28, 2020 15:39

Fix other isDismissable instances

699faaa

Merge branch 'master' into function-as-a-child-dialog-triggers

c0ea798

kill me, i hate typescript

a0e593f

finally fixed all the things

c6618ff

Add a test for isDismissable behaviors

57297a8

write tests for alert dialog

a74def2

ktabors reviewed Mar 2, 2020

View reviewed changes

Merge branch 'master' into function-as-a-child-dialog-triggers

8181837

LFDanLu reviewed Mar 3, 2020

View reviewed changes

packages/@react-spectrum/dialog/stories/Dialog.stories.tsx Outdated Show resolved Hide resolved

LFDanLu added 2 commits March 5, 2020 09:57

Addressing review comments

215eed5

Merge branch 'master' of https://github.com/adobe-private/react-spect…

c5ac3f5

…rum-v3 into function-as-a-child-dialog-triggers

LFDanLu mentioned this pull request Mar 5, 2020

(DNM) Making only dismissible modal dialogs closable via underlay click #256

Closed

5 tasks

Merge branch 'master' of https://github.com/adobe-private/react-spect…

5e196aa

…rum-v3 into function-as-a-child-dialog-triggers

LFDanLu added 2 commits March 5, 2020 16:01

fixing some of the types and missed merge stuff

b6ed84f

Making dismissible modals (ONLY) closable by underlay click

99fac46

Popover, tray, tooltip, etc should all still be dismissible by outside interaction click

LFDanLu added 3 commits March 5, 2020 16:55

adding tests for useOverlay closeOnInteractOutside

edb4f4d

Merge branch 'master' of https://github.com/adobe-private/react-spect…

ce2801e

…rum-v3 into function-as-a-child-dialog-triggers

fixing lint

5a934ce

ktabors reviewed Mar 7, 2020

View reviewed changes

updating test as per review

76773da

snowystinger commented Mar 9, 2020

View reviewed changes

Merge branch 'master' into function-as-a-child-dialog-triggers

fb92718

ktabors previously approved these changes Mar 9, 2020

View reviewed changes

MidnightCoder06 previously approved these changes Mar 9, 2020

View reviewed changes

devongovett reviewed Mar 9, 2020

View reviewed changes

Addressing review comments

528bb83

LFDanLu dismissed stale reviews from MidnightCoder06 and ktabors via 528bb83 March 9, 2020 23:11

Merge branch 'master' into function-as-a-child-dialog-triggers

e155974

devongovett approved these changes Mar 10, 2020

View reviewed changes

devongovett merged commit b42a6d5 into master Mar 10, 2020

devongovett deleted the function-as-a-child-dialog-triggers branch March 10, 2020 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add close through a child function to dialogs #234

Add close through a child function to dialogs #234

snowystinger commented Feb 28, 2020 •

edited by LFDanLu

Loading

codecov bot commented Feb 28, 2020 •

edited

Loading

adobe-bot commented Feb 29, 2020

adobe-bot commented Feb 29, 2020

adobe-bot commented Feb 29, 2020

ktabors Mar 2, 2020

snowystinger Mar 9, 2020

ktabors Mar 2, 2020

adobe-bot commented Mar 3, 2020

LFDanLu left a comment

adobe-bot commented Mar 5, 2020

adobe-bot commented Mar 5, 2020

adobe-bot commented Mar 6, 2020

adobe-bot commented Mar 6, 2020

ktabors Mar 7, 2020

LFDanLu Mar 9, 2020

adobe-bot commented Mar 9, 2020

snowystinger Mar 9, 2020

adobe-bot commented Mar 9, 2020

ktabors left a comment

devongovett left a comment

devongovett Mar 9, 2020

devongovett Mar 9, 2020

adobe-bot commented Mar 9, 2020

adobe-bot commented Mar 9, 2020

Add close through a child function to dialogs #234

Add close through a child function to dialogs #234

Conversation

snowystinger commented Feb 28, 2020 • edited by LFDanLu Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Team:

codecov bot commented Feb 28, 2020 • edited Loading

Codecov Report

adobe-bot commented Feb 29, 2020

adobe-bot commented Feb 29, 2020

adobe-bot commented Feb 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Mar 3, 2020

LFDanLu left a comment

Choose a reason for hiding this comment

adobe-bot commented Mar 5, 2020

adobe-bot commented Mar 5, 2020

adobe-bot commented Mar 6, 2020

adobe-bot commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Mar 9, 2020

Choose a reason for hiding this comment

adobe-bot commented Mar 9, 2020

ktabors left a comment

Choose a reason for hiding this comment

devongovett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Mar 9, 2020

adobe-bot commented Mar 9, 2020

snowystinger commented Feb 28, 2020 •

edited by LFDanLu

Loading

codecov bot commented Feb 28, 2020 •

edited

Loading