Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
theinterned opened this issue Apr 12, 2024 · 2 comments

Comments

@theinterned
Copy link

theinterned commented Apr 12, 2024

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
Copy link
Member

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
Copy link
Author

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants