Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c02b5fc
starting work on closable dialogs
snowystinger Feb 28, 2020
e574357
Works, now for types and dismiss button
snowystinger Feb 28, 2020
4f0b9a2
Merge branch 'master' into function-as-a-child-dialog-triggers
snowystinger Feb 28, 2020
c2fb8ed
update to yarn lock because apparently it didn't resolve correctly wh…
snowystinger Feb 28, 2020
70fa9ba
update a bunch of stories and add a test
snowystinger Feb 28, 2020
9af5066
Merge branch 'master' into function-as-a-child-dialog-triggers
snowystinger Feb 28, 2020
36216d6
Hook up dismissable
snowystinger Feb 28, 2020
ccd6901
appease typescript
snowystinger Feb 28, 2020
699faaa
Fix other isDismissable instances
snowystinger Feb 28, 2020
c0ea798
Merge branch 'master' into function-as-a-child-dialog-triggers
snowystinger Feb 28, 2020
a0e593f
kill me, i hate typescript
snowystinger Feb 28, 2020
c6618ff
finally fixed all the things
snowystinger Feb 29, 2020
57297a8
Add a test for isDismissable behaviors
snowystinger Feb 29, 2020
a74def2
write tests for alert dialog
snowystinger Feb 29, 2020
8181837
Merge branch 'master' into function-as-a-child-dialog-triggers
LFDanLu Mar 3, 2020
215eed5
Addressing review comments
LFDanLu Mar 5, 2020
c5ac3f5
Merge branch 'master' of https://github.com/adobe-private/react-spect…
LFDanLu Mar 5, 2020
5e196aa
Merge branch 'master' of https://github.com/adobe-private/react-spect…
LFDanLu Mar 5, 2020
b6ed84f
fixing some of the types and missed merge stuff
LFDanLu Mar 6, 2020
99fac46
Making dismissible modals (ONLY) closable by underlay click
LFDanLu Mar 5, 2020
edb4f4d
adding tests for useOverlay closeOnInteractOutside
LFDanLu Mar 6, 2020
ce2801e
Merge branch 'master' of https://github.com/adobe-private/react-spect…
LFDanLu Mar 6, 2020
5a934ce
fixing lint
LFDanLu Mar 6, 2020
76773da
updating test as per review
LFDanLu Mar 9, 2020
fb92718
Merge branch 'master' into function-as-a-child-dialog-triggers
snowystinger Mar 9, 2020
528bb83
Addressing review comments
LFDanLu Mar 9, 2020
e155974
Merge branch 'master' into function-as-a-child-dialog-triggers
LFDanLu Mar 9, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/@react-aria/overlays/src/useOverlay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import {useInteractOutside} from '@react-aria/interactions';
interface OverlayProps {
ref: RefObject<HTMLElement | null>,
onClose?: () => void,
isOpen?: boolean
isOpen?: boolean,
// Whether to close overlay if underlay is clicked
isDismissable?: boolean
}

interface OverlayAria {
Expand All @@ -26,7 +28,7 @@ interface OverlayAria {
const visibleOverlays: RefObject<HTMLElement>[] = [];

export function useOverlay(props: OverlayProps): OverlayAria {
let {ref, onClose, isOpen} = props;
let {ref, onClose, isOpen, isDismissable = false} = props;

// Add the overlay ref to the stack of visible overlays on mount, and remove on unmount.
useEffect(() => {
Expand Down Expand Up @@ -58,7 +60,7 @@ export function useOverlay(props: OverlayProps): OverlayAria {
};

// Handle clicking outside the overlay to close it
useInteractOutside({ref, onInteractOutside: onHide});
useInteractOutside({ref, onInteractOutside: isDismissable ? onHide : null});

return {
overlayProps: {
Expand Down
23 changes: 19 additions & 4 deletions packages/@react-aria/overlays/test/useOverlay.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,20 @@ describe('useOverlay', function () {
expect(document.activeElement).toBe(input);
});

it('should hide the overlay when clicking outside', function () {
it('should hide the overlay when clicking outside if isDismissble is true', function () {
let onClose = jest.fn();
render(<Example isOpen onClose={onClose} />);
render(<Example isOpen onClose={onClose} isDismissable />);
fireEvent.mouseUp(document.body);
expect(onClose).toHaveBeenCalledTimes(1);
});

it('should not hide the overlay when clicking outside if isDismissable is false', function () {
let onClose = jest.fn();
render(<Example isOpen onClose={onClose} isDismissable={false} />);
fireEvent.mouseUp(document.body);
expect(onClose).toHaveBeenCalledTimes(0);
});

it('should hide the overlay when pressing the escape key', function () {
let onClose = jest.fn();
let res = render(<Example isOpen onClose={onClose} />);
Expand All @@ -49,11 +56,19 @@ describe('useOverlay', function () {
expect(onClose).toHaveBeenCalledTimes(1);
});

it('should still hide the overlay when pressing the escape key if isDismissable is false', function () {
let onClose = jest.fn();
let res = render(<Example isOpen onClose={onClose} isDismissable={false} />);
let el = res.getByTestId('test');
fireEvent.keyDown(el, {key: 'Escape'});
expect(onClose).toHaveBeenCalledTimes(1);
});

it('should only hide the top-most overlay', function () {
let onCloseFirst = jest.fn();
let onCloseSecond = jest.fn();
render(<Example isOpen onClose={onCloseFirst} />);
let second = render(<Example isOpen onClose={onCloseSecond} />);
render(<Example isOpen onClose={onCloseFirst} isDismissable />);
let second = render(<Example isOpen onClose={onCloseSecond} isDismissable />);

fireEvent.mouseUp(document.body);
expect(onCloseSecond).toHaveBeenCalledTimes(1);
Expand Down
18 changes: 12 additions & 6 deletions packages/@react-spectrum/dialog/src/AlertDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,23 @@

import AlertMedium from '@spectrum-icons/ui/AlertMedium';
import {Button} from '@react-spectrum/button';
import {chain} from '@react-aria/utils';
import {classNames, useStyleProps} from '@react-spectrum/utils';
import {Content, Footer, Header} from '@react-spectrum/view';
import {Dialog} from './Dialog';
import {DialogContext, DialogContextValue} from './context';
import {Divider} from '@react-spectrum/divider';
import {Heading} from '@react-spectrum/typography';
import React from 'react';
import React, {useContext} from 'react';
import {SpectrumAlertDialogProps} from '@react-types/dialog';
import {SpectrumButtonProps} from '@react-types/button';
import styles from '@adobe/spectrum-css-temp/components/dialog/vars.css';

export function AlertDialog(props: SpectrumAlertDialogProps) {
let {
onClose = () => {}
} = useContext(DialogContext) || {} as DialogContextValue;

let {
variant,
children,
Expand All @@ -32,8 +38,8 @@ export function AlertDialog(props: SpectrumAlertDialogProps) {
autoFocusButton,
title,
isConfirmDisabled,
onCancel,
onConfirm,
onCancel = () => {},
onConfirm = () => {},
...otherProps
} = props;
let {styleProps} = useStyleProps(otherProps);
Expand All @@ -53,9 +59,9 @@ export function AlertDialog(props: SpectrumAlertDialogProps) {
<Divider size="M" />
<Content>{children}</Content>
<Footer>
{secondaryLabel && <Button variant="secondary" onPress={() => onConfirm('secondary')} autoFocus={autoFocusButton === 'secondary'}>{secondaryLabel}</Button>}
{cancelLabel && <Button variant="secondary" onPress={onCancel} autoFocus={autoFocusButton === 'cancel'}>{cancelLabel}</Button>}
<Button variant={confirmVariant} onPress={() => onConfirm('primary')} isDisabled={isConfirmDisabled} autoFocus={autoFocusButton === 'primary'}>{primaryLabel}</Button>
{secondaryLabel && <Button variant="secondary" onPress={() => chain(onClose(), onConfirm('secondary'))} autoFocus={autoFocusButton === 'secondary'}>{secondaryLabel}</Button>}
{cancelLabel && <Button variant="secondary" onPress={() => chain(onClose(), onCancel())} autoFocus={autoFocusButton === 'cancel'}>{cancelLabel}</Button>}
<Button variant={confirmVariant} onPress={() => chain(onClose(), onConfirm('primary'))} isDisabled={isConfirmDisabled} autoFocus={autoFocusButton === 'primary'}>{primaryLabel}</Button>
</Footer>
</Dialog>
);
Expand Down
8 changes: 4 additions & 4 deletions packages/@react-spectrum/dialog/src/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,16 @@ export function Dialog(props: SpectrumDialogProps) {
} = useContext(DialogContext) || {} as DialogContextValue;
let {
children,
isDismissable,
onDismiss,
isDismissable = contextProps.isDismissable,
onDismiss = contextProps.onClose,
...otherProps
} = props;
let {styleProps} = useStyleProps(otherProps);
let allProps: SpectrumBaseDialogProps = mergeProps(
mergeProps(
mergeProps(
filterDOMProps(otherProps),
contextProps
filterDOMProps(contextProps)
),
styleProps
),
Expand All @@ -56,7 +56,7 @@ export function Dialog(props: SpectrumDialogProps) {
return (
<ModalDialog {...allProps} size={size}>
{children}
{isDismissable && <ActionButton slot="closeButton" isQuiet icon={<CrossLarge size="L" />} onPress={onDismiss} />}
{isDismissable && <ActionButton slot="closeButton" isQuiet icon={<CrossLarge size="L" />} aria-label="dismiss" onPress={onDismiss} />}
</ModalDialog>
);
}
Expand Down
39 changes: 29 additions & 10 deletions packages/@react-spectrum/dialog/src/DialogTrigger.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import {DialogContext} from './context';
import {DOMRefValue} from '@react-types/shared';
import {Modal, Overlay, Popover, Tray} from '@react-spectrum/overlays';
import {PressResponder} from '@react-aria/interactions';
import React, {Fragment, useRef} from 'react';
import {SpectrumDialogTriggerProps} from '@react-types/dialog';
import React, {Fragment, ReactElement, useRef} from 'react';
import {SpectrumDialogClose, SpectrumDialogProps, SpectrumDialogTriggerProps} from '@react-types/dialog';
import {unwrapDOMRef, useMediaQuery} from '@react-spectrum/utils';
import {useControlledState} from '@react-stately/utils';
import {useOverlayPosition, useOverlayTrigger} from '@react-aria/overlays';
Expand All @@ -27,9 +27,14 @@ export function DialogTrigger(props: SpectrumDialogTriggerProps) {
mobileType = type === 'popover' ? 'modal' : type,
hideArrow,
targetRef,
isDismissable,
...positionProps
} = props;
let [trigger, content] = React.Children.toArray(children);
if (!Array.isArray(children) || children.length > 2) {
throw new Error('DialogTrigger must have exactly 2 children');
}
// if a function is passed as the second child, it won't appear in toArray
let [trigger, content] = children as [ReactElement, SpectrumDialogClose];

// On small devices, show a modal or tray instead of a popover.
// TODO: DNA variable?
Expand Down Expand Up @@ -66,20 +71,20 @@ export function DialogTrigger(props: SpectrumDialogTriggerProps) {
case 'fullscreen':
case 'fullscreenTakeover':
return (
<Modal isOpen={isOpen} onClose={onClose} type={type}>
{content}
<Modal isOpen={isOpen} isDismissable={false} onClose={onClose} type={type}>
{typeof content === 'function' ? content(onClose) : content}
</Modal>
);
case 'modal':
return (
<Modal isOpen={isOpen} onClose={onClose}>
{content}
<Modal isOpen={isOpen} isDismissable={isDismissable} onClose={onClose}>
{typeof content === 'function' ? content(onClose) : content}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</Modal>
);
case 'tray':
return (
<Tray isOpen={isOpen} onClose={onClose}>
{content}
{typeof content === 'function' ? content(onClose) : content}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</Tray>
);
}
Expand All @@ -91,6 +96,7 @@ export function DialogTrigger(props: SpectrumDialogTriggerProps) {
isOpen={isOpen}
onPress={onPress}
onClose={onClose}
isDismissable={isDismissable}
trigger={trigger}
overlay={renderOverlay()} />
);
Expand All @@ -111,7 +117,7 @@ function PopoverTrigger({isOpen, onPress, onClose, targetRef, trigger, content,
shouldFlip: props.shouldFlip,
isOpen
});

let {triggerAriaProps, overlayAriaProps} = useOverlayTrigger({
ref: triggerRef,
type: 'dialog',
Expand Down Expand Up @@ -145,10 +151,23 @@ function PopoverTrigger({isOpen, onPress, onClose, targetRef, trigger, content,
);
}

function DialogTriggerBase({type, isOpen, onPress, onClose, dialogProps = {}, triggerProps = {}, overlay, trigger}) {
interface SpectrumDialogTriggerBase {
type?: 'modal' | 'popover' | 'tray' | 'fullscreen' | 'fullscreenTakeover',
isOpen?: boolean,
onPress?: any,
onClose?: () => void,
isDismissable?: boolean
dialogProps?: SpectrumDialogProps | {},
triggerProps?: any,
overlay: ReactElement,
trigger: ReactElement
}

function DialogTriggerBase({type, isOpen, onPress, onClose, isDismissable, dialogProps = {}, triggerProps = {}, overlay, trigger}: SpectrumDialogTriggerBase) {
let context = {
type,
onClose,
isDismissable,
...dialogProps
};

Expand Down
3 changes: 2 additions & 1 deletion packages/@react-spectrum/dialog/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
import React, {HTMLAttributes} from 'react';

export interface DialogContextValue extends HTMLAttributes<HTMLElement> {
type: 'modal' | 'popover' | 'tray',
type: 'modal' | 'popover' | 'tray' | 'fullscreen' | 'fullscreenTakeover',
isDismissable?: boolean,
onClose?: () => void
}

Expand Down
Loading