Skip to content

Toast 🍞#3868

Merged
devongovett merged 23 commits into
mainfrom
toast
Jan 12, 2023
Merged

Toast 🍞#3868
devongovett merged 23 commits into
mainfrom
toast

Conversation

@devongovett
Copy link
Copy Markdown
Member

🎄🎁🌟 Happy holidays! 🌟🎁🎄

This implements the Toast component in React Spectrum and React Aria. It's implemented as a labeled landmark region using our existing landmark hooks for keyboard navigation. Following the Spectrum guidelines, a single toast is displayed at a time, with a priority queue system. In React Aria, the visible toast limit is configurable.

There is a global queue of toasts that toasts are added to, and a <ToastContainer> component to render them. Only one ToastContainer will render at a time, even if an app renders more than one. If one ToastContainer unmounts, another one will take over displaying the toasts if rendered somewhere else. This is so components such as those in quarry can render their own ToastContainer and not depend on the app providing one. But if multiple components do so, the toasts are still only displayed once.

Users can navigate to the toast region using landmark navigation (F6). When a toast closes, focus is moved to the next toast in the queue if any. Otherwise, it moves back to wherever focus was last before entering the toast container.

We ensure that focus management works, and toasts are never aria-hidden, even when containing FocusScopes such as dialogs are open. This is implemented by assigning a data-react-aria-top-layer attribute to the toast region element. Elements with this attribute will not be aria-hidden by our utilities, always allow focus to them even when not inside a containing FocusScope, and will not cause overlays to close when clicking on them.

This will be complicated somewhat by apps like Unified Shell which use iframes. That breaks landmark navigation, and also may cause toasts outside the frame to overlap with those inside the iframe rather than being merged into a single queue. To handle this, when a toast is queued, a custom DOM event is emitted on the window. Shell or others could handle this event and call preventDefault on it. If that happens, the toast is not added to the local queue. Shell would be responsible for post messaging out of the iframe and re-queuing the toast outside. They would also be responsible for managing focus to navigate both out and back into the frame.

@devongovett
Copy link
Copy Markdown
Member Author

Devonbot:

@devongovett
Copy link
Copy Markdown
Member Author

Comment thread packages/@adobe/spectrum-css-temp/components/toast/index.css
Comment thread packages/@react-aria/focus/src/FocusScope.tsx
Comment thread packages/@react-aria/toast/src/useToastRegion.ts
Comment thread packages/@react-aria/toast/src/useToastRegion.ts Outdated
Comment thread packages/@react-stately/toast/src/useToastState.ts
Comment thread packages/@react-spectrum/toast/docs/Toast.mdx Outdated
Comment thread packages/@react-spectrum/toast/docs/Toast.mdx Outdated
Comment thread packages/@react-spectrum/toast/docs/Toast.mdx Outdated
Comment thread packages/@react-spectrum/toast/docs/Toast.mdx Outdated
Comment thread packages/@react-spectrum/toast/docs/Toast.mdx Outdated

const SpectrumToastQueue = {
/** Queues a neutral toast. */
neutral(children: ReactNode, options: SpectrumToastOptions = {}): CloseFunction {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we limit children to type string?

Comment thread packages/@react-spectrum/toast/stories/Toast.stories.tsx Outdated
Comment thread packages/@react-spectrum/toast/stories/Toast.stories.tsx Outdated
Comment thread packages/@react-spectrum/toast/stories/Toast.stories.tsx Outdated
snowystinger
snowystinger previously approved these changes Jan 11, 2023
Copy link
Copy Markdown
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Tested in Chrome and Safari w/ VO, working as expected

Copy link
Copy Markdown
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Partial review, looking at the spectrum docs/components next. Happy for these to be followup.

Note: testing on landmark-iframe branch, commenting here for context

Comment thread packages/@react-aria/toast/docs/useToast.mdx
Comment thread packages/@react-aria/toast/docs/useToast.mdx
Comment thread packages/@react-aria/toast/src/useToast.ts Outdated
Comment on lines +93 to +94
'aria-label': props['aria-label'],
'aria-labelledby': props['aria-labelledby'] || titleId,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do these need to go through useLabels to construct a aria-labelledby that announces the user provided aria labeling + the title/description of the toast itself? Perhaps a non-issue/minor point, not sure when someone would have a toast title and still provide a aria-label/aria-labelledby

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.

Maybe. TBH I'm not sure if this is even working. The goal was to prevent screen readers from announcing the close button after reading the content of the toast (by pointing aria-labelledby directly to the title), but in my testing VoiceOver at least still reads everything. So this might not actually do anything at all. Further testing in other screen readers needed. Will handle later.

Comment on lines +46 to +49
onBlurWithin: () => {
state.resumeAll();
lastFocused.current = null;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Random thought: what if the user changes windows while focused inside the toast? I kinda feel like that could be a case where the toast timers should pause, but not sure there is a great way to distinguish between that scenario and if focus is being lost to the body (both have relatedTarget = null).

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.

there are page visibility events we could use but not gonna do that for this PR :D

Comment thread packages/@react-aria/toast/src/useToastRegion.ts
Comment thread packages/@react-stately/toast/src/useToastState.ts Outdated
Comment thread packages/@react-aria/toast/docs/useToast.mdx Outdated
state: ToastState<unknown>
}

export function Toaster(props: ToastContainerProps): ReactElement {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🍞 🔥 🤣

import {SpectrumToastValue, Toast} from './Toast';
import {Toaster} from './Toaster';
import {ToastOptions, ToastQueue, useToastQueue} from '@react-stately/toast';
import {useSyncExternalStore} from 'use-sync-external-store/shim';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a comment about why we are using this shim? Either here, in its usage or in the stately hook

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.

Added in stately.

Comment thread packages/@react-stately/toast/src/useToastState.ts
Co-authored-by: Daniel Lu <dl1644@gmail.com>
Copy link
Copy Markdown
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Approving, will post any followup review comments later

EDIT: no other comments, things look great!

@devongovett devongovett merged commit e023016 into main Jan 12, 2023
@devongovett devongovett deleted the toast branch January 12, 2023 18:41
@devongovett devongovett restored the toast branch January 12, 2023 18:42
@devongovett devongovett deleted the toast branch January 12, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants