Add transition prop to DialogPanel and DialogBackdrop components
#3309
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This will allow us to reset the `OpenClosedProvider` and reset the
"boundary". This is important when we want to wrap a `Dialog` inside of
a `Transition` that exists in another component that is wrapped in a
transition itself.
This will be used in let's say a `DisclosurePanel`:
```tsx
<Disclosure> // OpenClosedProvider
<Transition>
<DisclosurePanel> // ResetOpenClosedProvider
<Dialog /> // Can safely wrap `<Dialog />` in `<Transition />`
</DisclosurePanel>
</Transition>
</Disclosure>
```
This prop allows us to enabled / disable the `Transition` functionality. E.g.: expose the underlying data attributes. But it will still setup a `Transition` boundary for coordinating the `TransitionChild` components.
+ add `transition` props to the `Dialog`, `DialogPanel` and `DialogBackdrop` This will allow us individually control the transition on each element, but also setup the transition boundary on the `Dialog` for coordination purposes.
This technically means most or all of them can be removed from InternalDialog but we can do that later
I’m not 100% sure this is right but it seems like it might be given that `static` implies it’s always rendered.
Already validated by `Dialog` itself
The reason this test is flawed and why it's safe to delete it: This test opened the dialog, then clicked on an element outside of the dialog to close it and prove that we correctly focused that new element instead of going to the button that opened the dialog in the first place. This test used to work before marked the rest of the page as `inert`. Right now we mark the rest of the page as `inert`, so running this in a real browser means that we can't click or focus an element outside of the `dialog` simply because the rest of the page is inert. The reason it fails all of a sudden is that the introduction of `<Transition>` around the `<Dialog>` by default purely delays the mounting just enough to record different elements to try and restore focus to. That said, this test clicked outside of a dialog and focused that element which can't work in a real browser because the element can't be interacted with at all.
RobinMalfait
force-pushed
the
feat/transition-dialog-backdrop-and-panel
branch
from
393949d
to
deaf348
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a continuation of #3307
In #3307 we introduced the
transitionprop on theDialogcomponent. But to make the API more consistent with the other components we want to be explicit and add thetransitionprop to the elements where the data attributes are being used.This means that instead of enabling it on the
Dialogdirectly, you add thetransitionprop to the individual components:It's only needed on the
Dialogcomponent directly if you also want to transition this component.