-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
DropdownMenuV2: update animation #64868
Conversation
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @nick-a8c. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible we broke the use case in a modal?
Here's what I'm seeing with this PR:
Screen.Recording.2024-08-28.at.20.41.38.mov
and with trunk
, things work well:
Screen.Recording.2024-08-28.at.20.43.56.mov
10d7ae9
to
788066b
Compare
Nice find — the latest commit should fix it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM and tests well for me 👍 🚀
Left a few minor optional suggestions.
Do we need a design 👁️ on it before merging?
788066b
to
04c9924
Compare
I addressed all of @tyxla 's feedback.
I can also reproduce on my machine in the site editor, but I can't reproduce in Storybook — it feels like a browser glitch, and I'm unsure how much we can do about it. I'm not sure if this is somehow related to ariakit/ariakit#4083.
Talking with @nick-a8c, we put the specs and the real animation side-to-side, and the only difference we could notice was in the outer wrapper scale factor — which I now updated to hopefully match spec even better. Regarding the animation duration, I guess the tricky part is to find a length that feels good for both small and large menus. The larger the menu, the more distance it needs to travel. Therefore, this same animation will potentially feel snappier for larger menus, and slower for smaller menus. If we pick a duration that is too short, the animation may feel too fast for large menus. If we pick a duration that is too long, the animation may feel too slow for smaller menus. IMO it's a matter of choosing the right compromise, and I think you and @nick-a8c are the best people to pick a value. You can test different animation settings by tweaking these values — let me know once you settle on a duration, and I can update the PR and merge it. |
Thank you, @ciampo, for applying the edits! Regarding the balance for animation duration between small and large menus, I was thinking we could run a test using the exact same elements. This way, we can ensure the motion matches perfectly both in After Effects and CSS. We could use the same menu content and length for the test, creating two versions—one with a longer menu and one with a shorter menu. This would help us see how the animation behaves with different content lengths and allow us to find that ideal duration for both cases. @jameskoster, do you have two menus in mind—one short and one long—so I can apply the AE motion to them? Then Marco can use the same menus to match and compare the movement later. |
You could likely use the Storybook examples — there are menus of different lengths. |
I'm happy to defer to Nick on the duration :) |
04c9924
to
bd6d70b
Compare
Pushed a little update that hopefully will help with the glitchiness flagged by Jay above.
Sounds good to me — @nick-a8c, feel free to suggest a different duration, otherwise I guess we can merge the PR as-is |
Flaky tests detected in 76d217e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10664369751
|
This is looking good! Do we have exit animations planned for them? |
I'm not aware of any exit animation specs, but I'm happy to discuss them with Jay and Nick. |
@mtias I’m currently suggesting a fade-out (100ms), as shown in the video example. I’m not sure if an exit animation is necessary, but I’ll gather more information and present a few solutions in the next 'Site Editor Motion' update. I’m also interested in hearing @jameskoster's thoughts on this. feedback-01.mp4 |
bd6d70b
to
75b70aa
Compare
Added fade-out exit animation, in case you wanted to test in in Storybook / the editor ddmv2.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exit animation works great in my testing 👍
- Refactor from CSS animation to transition - Use `[data-enter]` and `[data-leave]` ariakit attributes - Move CSS comments inline
Co-authored-by: Marin Atanasov <[email protected]>
fad29d8
to
c26c72e
Compare
I'm going to merge this PR with the current specs — we can always iterate with tweaks as needed. |
Awesome! The exit animation helps quite a bit, let's see how it goes. |
@nick-a8c would you mind sharing your WordPress.org username so we can ensure you're properly credited in the 6.7 release next week? |
What?
Part of #50459
Updates
DropdownMenuV2
animation following the latest specs (shared by @nick-a8c)Why?
For a better sense of polish, and alignment with the rest of the animations in Gutenberg.
How?
The new animation specs require the popover wrapper and the popover contents to scale at different rates. That required a small refactor to include an extra wrapper element, to account for the two separate animations (proof of concept: https://codepen.io/ciampo/pen/mdZjPWg)
The scale animation does not happen when the popover opens to the side of its trigger — in that case, the popover only fades in.
Testing Instructions
Check
DropdownMenuV2
examples in Storybook and its usages across the editor, make sure the animation looks good and follows the specs, without impactive the usability and the semantics of the component.-->Screenshots or screencast
ddmv2.new.anim.spec.mp4