Modal: add exit animation #65203
Modal: add exit animation #65203
Conversation
ciampo
force-pushed
the
feat/modal-exit-animation
branch
from
514bc9b to
5d04a1b
Compare
ciampo
added
[Type] Enhancement
|
Size Change: +544 B (+0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
feat/modal-exit-animation
branch
from
5d04a1b to
799f002
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ciampo
added
Needs Design Feedback
ciampo
force-pushed
the
feat/modal-exit-animation
branch
from
799f002 to
25c95c3
Compare
|
@WordPress/gutenberg-design For now, I've reversed the enter animation, and only "softened" the scale transform a bit. Happy to hear your feedback on this one. |
|
This makes such a big difference 😍 Here are the specs for the outro animation I saw @nick-a8c share elsewhere: Is there a way to apply the exit animation when Cancel buttons are clicked? Or to set up the component so that consumers can reference the close function that triggers the animation and apply that to Cancel buttons? Hopefully that makes sense 😅 |
ciampo
force-pushed
the
feat/modal-exit-animation
branch
from
25c95c3 to
011e4e1
Compare
|
Exit animation spec updated.
Unfortunately no, and that's because of how In particular, the way the modal is shown/hidden expects the consumer of { isModalOpen ? <Modal {...} /> : null }Because of this, The way to overcome this limitation would be to add something like an <Modal open={ isModalOpen } />This way, the
I see what you're suggesting, but that would be a poor (and brittle) way of implementing it. I understand that having the exit animation play only for certain interactions is not great, and in fact my personal advice is to keep The correct way is the one illustrated above, which I believe can't be done without introducing a breaking change to the existing version of (cc @WordPress/gutenberg-components ) |
|
Flaky tests detected in bbb49a7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10922464472
|
Because of the inconsistent exit animations? It's a shame, but I don't know if it needs to block what is otherwise a nice improvement. Is it worth opening an issue the discuss the new component? |
Having an inconsistent behavior (sometimes animated, sometimes not animated) could hurt the UX, giving the impression that things are not working as expected.
I left a comment in the original |
There was a problem hiding this comment.
This tests great, and the code looks good. 👍
I've left some minor suggestions, and I mostly prefer if the animation changes could be in their own PR. That's mostly because we're deprecating an animation, but also because it's a good change to split as an atomic commit.
FWIW I'd be good to ship this regardless, as it's a great piece of polish 🚀
ciampo
force-pushed
the
feat/modal-exit-animation
branch
2 times, most recently
from
ba25974 to
f1b1a30
Compare
| const frameEl = frameRef.current; | ||
|
|
||
| if ( ! frameEl ) { | ||
| closeModalReject(); |
There was a problem hiding this comment.
I think that rejecting is too harsh 🙂 This will most likely happen when the programmer who uses the hook makes a mistake and doesn't pass the ref to an element. Or if the element is mounted conditionally or with some delay. Then we should print a console warning ("you're using the hook wrong"), but it shouldn't prevent the modal from closing at all.
There was a problem hiding this comment.
I changed from rejecting to warning + resolving. Let me know in case this wasn't what you had in mind
There was a problem hiding this comment.
Yes, that's what I had in mind. If we don't have any element to animate, it's bad and probably there's some bug somewhere, but it's not a reason to abort closing the modal.
ciampo
force-pushed
the
feat/modal-exit-animation
branch
from
0ab921d to
d774215
Compare
|
@ciampo I have some situations where I don't want the modal to close (there is a dialog that asks if the user wants to save changes) and the animation makes it quite jarring. chrome_trIu209da6.mp4 |
|
To clarify my thoughts a bit on this after testing some more:
|
|
Hey @rmorse , thank you for your feedback — it's all very sensible. Unfortunately, the current implementation of the component doesn't allow for fine-grain control on the exit animation (as explained more in detail above); the I can't think of many alternatives beyond exposing a prop to disable animations on the Moving to a completely new paradigm (ie. creating a new version of the component), like explained above, will allow us to change the API surface and introduce more granular animation behavior. |
|
Coming at this from mostly a design system angle, I'm trying to understand the particular use case, which is a modal that contains a save/update button that both closes the modal, and takes you to a different URL that requires you to save your document because it's outside the editor. That's from viewing the video example (thank you for providing!)—Is that right? I think I need to more fully understand the motivation for the particular design, because I feel like I'm missing some very important nuance. Just at a glance, it seems like if that is the desired flow, the update button should also save the document before navigating elsewhere, avoiding the confirm firing. Or something else entirely. Animation on components in general will obviously be disabled if the user has set the |
After watching the video closely, I see something different: the modal has an "Update" button that saves the changes done in the modal and closes the modal. And there's also the Close (x) button that closes the modal without saving. The browser What strikes me as odd is not the animation itself, but that the modal closes before the |
The
@jsnajdr do you have a way to achieve this functionality with the current set of constraints (see this comment) ? @rmorse , would you be able to open a PR with a test Storybook example (this file) for the |
|
@jasmussen the modal + confirmation dialogue doesn't lead you anywhere else, you stay the on the same page - I like to think of it as editing an external resource that is not connected to the specific page, but some elements on the page are connected to it... so we provide a modal for editing so you don't need to leave the editor. The confirm dialog is really just a lazy implemenation that will be replaced by a custom element in the future - but as we're used to seeing it when navigating away from a page (with unsaved changes) I can see how it looks like that. If you press "cancel" on the confirm, then you are represented with the modal and your unsaved changes, you can then "cancel" to close without saving the modal or "save" to save the contents (but it doesn't close the modal) @ciampo will do - I'll try to get a PR up for this in the next few days - I think the issue will be easy to replicate and doesn't require adding a confirm dialogue, the only thing that needs to happen is that Thanks for looking into this! I thought I was going to have duplicate yet another WP component into my project and hack at it 😅 |
I can't promise we'll find a working solution, but we can at least look into it.
That's unfortunate to hear. As someone who spends the majority of their time working on the |
|
Here is a backward compatible way I can think of to allow preventing a close request: #67635 |
What?
Follow-up to #64580
Add an exit animation to the
Modalcomponent.Why?
For a better sense of polish.
How?
By adding an additional CSS animation, and waiting for it to end before invoking the
onRequestClosecallback prop, which consumers ofModaluse to conditionally render the component.Testing Instructions
Modalcomponent in Storybook and across the editorScreenshots or screencast
Kapture.2024-09-10.at.15.07.07.mp4