Allow closed state to be passed to Dialog for proper transition handling

[acelaya]

This PR introduces some changes on how the Dialog handles closed state, so that it can be passed to it for proper transition handling, but in a way that we can dynamically render the Dialog itself from consumer code, if a TransitionComponent is not provided.

Note
Most of the code changes here are because of the introduction of a prototype page to experiment with the new capabilities in Dialog.

How to use it

Up until now, we had to do something like this:

const [closed, setClosed] = useState(true);

return (
  <>
    {!closed && (
      <Dialog
        title="Dialog"
        onClose={() => setClosed(true)}
        transitionComponent={Slider}
      >
        Hello world
      </Dialog>
    )}
  </>
);

This works, as long as setClosed(true) is not called from outside the Dialog itself.

01.-.broken.webm

In order to fix that, a new closed prop is introduced, letting the Dialog itself both wrap onClose, but also react to changes to closed, and properly handle transitions in both cases:

const [closed, setClosed] = useState(true);

return (
  <Dialog
    title="Dialog"
    closed={closed}
    onClose={() => setClosed(true)}
    transitionComponent={Slider}
  >
    Hello world
  </Dialog>
);

02.-.fixed.webm

When no transitionComponent is provided, then both options above are valid.

03.-.no-transition.webm

Note
All examples can be tested by checking out this branch and going to http://localhost:4001/transition-components-experiments.

State change flow

The way this works is currently a bit messy (I want to put some extra thought on it), as it requires deriving two pieces of state from the new closed prop.

The reason we cannot use the prop "directly" is that the prop might change sooner, but we want to delay the state to actually change, if there's a transition involved.

These are the three pieces of state involved:

• closed: The prop that is passed from consuming code.
• transitionComponentVisible: Triggers in/out transition animations if a TransitionComponent is provided.
• isClosed: Prevents children to be rendered when true. Set to true after closed is set to true, but with a "delay" in case of transition.

This explains how the different combinations of prop and state flows work:

With TransitionComponent

`closed` prop set to `true` 
└── immediately set `transitionComponentVisible` to `false`
    └── `out` animation triggers
        └── after animation ends (see `onTransitionEnd`), set `isClosed` to `true`
            └── children are not rendered anymore

`closed` prop set to `false`
└── immediately set `isClosed` to `false`
    ├── children are rendered
    └── immediately set `transitionComponentVisible` to `true`
        └── `in` animation triggers

Without TransitionComponent

`closed` prop set to `true`
└── immediately set `isClosed` to `true`
    └── children are not rendered anymore

`closed` prop set to `false`
└── immediately set `isClosed` to `false`
    └── children are rendered

The conclusion is that from the 4 possible combinations, only the first one justifies the need to "derive" isClosed state from closed prop.

Considerations / thoughts / next steps

• isClosed feels like a too vague name for the internal state. It also might be confused with the closed prop.
• close and isClosed are true when not visible, while transitionComponentVisible is true when visible. Perhaps we should make the three consistent, maybe renaming to transitionComponentHidden.
• Removing the if (isClosed) return null logic simplifies the rest of the interactions, but leaves some DOM nodes for preact to handle forever. Scratch that. Without that check, then the close prop does nothing when no transitionComponent is provided, which would make it inconsistent.
• I think there should be a way to combine isClosed and transitionComponentVisible, but haven't nailed it yet.
• If we are planning to support transition components somewhere other than the Dialog, we may want to extract some of the logic, but I would start by copy-pasting once, and then it will be easier to identify what needs to be reused.

[lyzadanger]

Minor nit, but I typically import from the package entry point in pattern library page component modules.

[lyzadanger]

Thanks for putting so much thought into this! I think I generally grok where you're coming from.

If we are planning to support transition components somewhere other than the Dialog

We definitely are, and very soon, in fact. If it helps to consider another use case: we're going to want transitions on toast messages. The new Callout component can be thought of as a (rough) replacement for ToastMessageItem in https://github.com/hypothesis/client/blob/3bf184abfe1ed828c6cc7a903969b9f1d67d0362/src/shared/components/BaseToastMessages.tsx and I am considering modeling a shared toast-messages component after the BaseToastMessages component here. Right now the client BaseToastMessages component applies transitions but not using a TransitionComponent. Might want to give this a think. Not even certain at this point whether the transition belongs on the individual toast messages or the toast-message container. Would love some thoughts and input!

[acelaya]

Thanks for the feedback @lyzadanger

@robertknight made an interesting suggestion on trying to improve the internal sync of the three pieces of state:

Could we model the internal state as a flag indicating whether a transition is active or not?

isClosed = closed && !transitionActive
transitionComponentVisible = !closed || transitionActive

This goes in line with one of my TODOs above: I think there should be a way to combine isClosed and transitionComponentVisible, but haven't nailed it yet., but every attempt ends up breaking at least one of the combinations, so I think I will postpone this.

I'm adding a comment in the code to make sure we get reminded from time to time.

I may also try to play a bit with extracting the transition component handling into a hook that can be reused in other components:

const { closeHandler, Wrapper } = useTransitionComponent(transitionComponent, onClose, closed, ...);

I think isolating that from the rest of the logic in Dialog will also be helpful to identify how to simplify the internal state management, while we get ready for other components that need transitions.

[codecov]

Codecov Report

Merging #1117 (c29884d) into main (bfb0bdb) will not change coverage.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1117   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           57        57           
  Lines          806       822   +16     
  Branches       312       320    +8     
=========================================
+ Hits           806       822   +16     

Files Changed	Coverage Δ
src/components/data/AspectRatio.tsx	100.00% <ø> (ø)
src/components/data/Scroll.tsx	100.00% <ø> (ø)
src/components/data/ScrollContainer.tsx	100.00% <ø> (ø)
src/components/data/Table.tsx	100.00% <ø> (ø)
src/components/data/TableBody.tsx	100.00% <ø> (ø)
src/components/data/TableCell.tsx	100.00% <ø> (ø)
src/components/data/TableFoot.tsx	100.00% <ø> (ø)
src/components/data/TableHead.tsx	100.00% <ø> (ø)
src/components/data/TableRow.tsx	100.00% <ø> (ø)
src/components/feedback/Modal.tsx	100.00% <ø> (ø)
... and 30 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[acelaya]

This is a bit random, but the test was passing locally and failing in CI. I guess there's a chance there's a timing issue/race condition here, as we are talking about 10ms.

[acelaya]

This stub TransitionComponent was triggering this timeout on every render, which was not relevant for a component that was previously mounted and unmounted every time.

With the new logic to open/close, the component can render multiple times, triggering the setTimeout on each one of them. Hence, the addition of the useEffect.

[acelaya]

I may also try to play a bit with extracting the transition component handling into a hook that can be reused in other components

I have created a draft for this #1131

It still has the complexities introduced here, but at least they are all wrapped in a reusable hook instead of complicating DIalog.

It also allows to use transitions in other components if we want.

[acelaya]

I think there should be a way to combine isClosed and transitionComponentVisible, but haven't nailed it yet

After multiple attempts, I came to realize this is not possible, because the different state changes are not "linear", or "instant" when a transition component is provided. There are potential "delays" between the change of one piece of state and the next one.

I have slightly simplified it though, removing nested conditionals, so I think it's ready for a review again.

[acelaya]

After giving this a lot of thought, we have come to the conclusion that this approach introduces too much complexity to solve a niche use-case.

We'll probably try something else. Once of the options is using useImperativeHandle to expose the "wrapped" close handler, so that consumers can close the dialog with the transition.

This approach would expose a bit the transition handling implementation details, but make it much simpler to maintain in the long term.