Dialog should allow wrapping content, header, and footer (for example in Suspense or some context)

[theinterned]

Description

Currently the API for Dialog does not support an ergonomic way to wrap the full contents of the dialog (header, footer, and body) in a wrapper such as a Suspense boundary, context provider, error boundary etc. These all seem like legitimate use cases!

In particular it is a very common pattern to want to lazy-load the content (header, body, and footer) of a dialog along with providers (eg. relay environment, or other context) only when the dialog is opened. This would require providing a "connected content" (header, body, footer) as a lazy component, while providing a "dumb content" (header, body, footer) as fallback to a Suspense boundary.

💬 Additional context in slack thread

Steps to reproduce

This is a simplified example of the very common pattern of wanting to lazy-load a dialog within a suspense boundary

// THIS DOESN'T WORK
const LazyLoadedDialog = React.lazy(() => import('./MyConnectedDialog'));

<Suspense fallback={() => <Dialog title="loading"><Spinner /></Dialog>} />
  <LazyLoadedDialog />
</Suspense>

However the above doesn't work: if one were to wrap the entire dialog in a suspense boundary, the animation on the Backdrop will play as suspense transitions between the fallback and real dialogs, causing jarring animation between fallback and loaded states.

Desired solution

Separate the Dialog.Window from the Dialog.Contents — Split the Dialog component into two components with separate roles:

1. Dialog.Window: simply renders the Portal, Backdrop, and StyledDialog. Receives the role, width, height, position props.
2. Dialog.Content: renders the content (header, footer, body) and all the other props. Manages state, event handlers etc.

This would provide an API that allows wrapping all content within the Portal, Backdrop, and StyledDialog (Dialog.Window). This Dialog.Window can thus remain stable and open on the page while transitioning between states in a suspense boundary that wraps the Dialog.Content.

This would allow us to do something like:

// MyConnectedDialogContent.tsx
import {Dialog} from "primer/experimental/dialog";
import {useMyExpensiveState, MyExpensiveStateContext} from './MyExpensiveStateContext';
import {MyExpensiveFormThatRequiresStateContext} from './MyExpensiveFormThatRequiresStateContext'

export default function MyConnectedDialogContent({onClose}) {
  return (
    <ExpensiveStateContext>
      <MyDialogContent onClose={onClose} />
    </ExpensiveStateContext>
  )
}

function MyDialogContent({onClose}) {
  const {myExpensiveState,  doMyExpensiveThing, resetState} = useMyExpensiveState()

  return (
    <Dialog.Content
      onClose={() => {
        resetState()
        onClose()
      }
      renderHeader={() => <Dialog.Header>{myExpensiveState.title}<Dialog.Header/>}
      footerButtons={[{buttonType: primary, content: 'Save', onClick:  doMyExpensiveThing}]}
    >
      <MyExpensiveFormThatRequiresStateContext />
    </Dialog.Content>
  )
}

// MySuspendedDialog.tsx
import {useState, Suspense, lazy} from 'react';
import {Dialog} from "primer/experimental/dialog";
import {Spinner} from "primer/react"

const LazyMyConnectedDialogContent = lazy(() => import('./MyConnectedDialogContent'))

function MyFallbackDialogContent({onClose}) {
  <Dialog title="Loading Dialog" onClose={onClose}><Spinner /></Dialog>
}


export function MySuspendedDialog() {
  const [isOpen, setIsOpen] = useState(false);
  function onClose(){
    setIsOpen(false)
  }

  return open && (
    <Dialog.Window> // this just renders the `Portal, `div.Dialog__Backdrop` and `div.Dialog__StyledDialog` so that its stable between suspensions
      <Suspense fallback={<MyFallbackDialogContent onClose={onClose} />}>
        <LazyMyConnectedDialogContent onClose={onClose} />
      </Suspense>
    </Dialog.Window>
  )
}

Version

v36.12.0

Browser

No response

[keithamus]

I'm curious if you're able to suspend just the dialog contents, or if you feel like you need to suspend the dialog's title and other content?

Ideally, Dialogs should be present on the page, with a title, to avoid AT users pressing a button and either:

• not receiving immediate feedback that a dialog has opened (either because the dialog is not on the page, not open, or there is a boundary waiting to fetch the dialog to show it).
• receiving feedback that focus has changed, but not having a title to announce

I'm curious, would you be able to construct a dialog that is always available on the page and the suspense boundary is just the Dialog contents?

[theinterned]

The issue is that things like buttons need to be wired up to context that I want to be able to lazy load onto the page.

I do want to be able to provide a fallback header, footer, and content in order to provide immediate feedback (see the first example in the description, or the example in the slack): I just need the ability to lazy load content to swap in for the header, footer, and content.

Today it is easy enough to lazy load the content. It's really the header and footer that need a new API.

Does that make sense @keithamus ?

[lesliecdubs]

👋 Hi, I'm catching up after leave and came across this issue.

In particular it is a very common pattern to want to lazy-load the content (header, body, and footer) of a dialog along with providers (eg. relay environment, or other context) only when the dialog is opened. This would require providing a "connected content" (header, body, footer) as a lazy component, while providing a "dumb content" (header, body, footer) as fallback to a Suspense boundary.

@theinterned are there any instances of this pattern required/in progress that you are aware of? If not, can you share more about how/why/when this came up for you? Trying to gather some more context for informed prioritization.

[theinterned]

@lesliecdubs Yes! This came up in the "create issue" dialogue linked from the global nav.

I have an issue to track it here https://github.com/github/web-systems/issues/2061 and there's lots of detail in the PR linked from that issue https://github.com/github/github/pull/320833.

[lesliecdubs]

@theinterned thank you! Is that part of the global nav already shipped? If not, do you know if there's a planned date for going live?

[lesliecdubs]

It's really the header and footer that need a new API.

@theinterned can you share more about the use case for wrapping the header in Suspense? It seems like the "Create new issue" header would stay consistent, but let us know what we're missing.