ToggleGroupControl: rewrite backdrop animation with framer motion shared layout animations #50278
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
|
Size Change: +1.49 kB (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Enhancement
|
Next steps / ideas:
(cc @youknowriad @mirka @aaronrobertshaw @andrewserong @stokesman in case you either have capacity to experiment with this, or have other ideas) |
ciampo
force-pushed
the
feat/toggle-group-control-framer-motion-shared-layout
branch
from
c4fdc65
to
829e4e0
Compare
|
Update: I should have managed to solve the remaining issues. Regarding the backdrop not animating correctly inside Regargind the backdrop not animating correctly when The last standing issue now is around the "deselectable" version of
FInally, after the update to |
ciampo
commented
Jun 8, 2023
| export { | ||
| __unstableMotion, | ||
| __unstableAnimatePresence, | ||
| __unstableMotionContext, |
There was a problem hiding this comment.
TODO: we may want to export the motion context differently? (ie. stable, or as private API)
There was a problem hiding this comment.
Let's export all the providers (style, components, toolbar context, motion) consistently the same way, as private APIs.
There was a problem hiding this comment.
I'm not sure we can remove existing exports from the package — even if they are prefixed with __unstable, removing them would cause breaking changes in plugin and themes using them (I ran a quick search on wpdirectory and there are unfortunately some matches).
@youknowriad do you have any suggestions on how we may want to approach this? Given how "de facto" stable are __unstableMotion and __unstableAnimatePresence, should we just export them without the __unstable alias too?
ciampo
force-pushed
the
feat/toggle-group-control-framer-motion-shared-layout
branch
2 times, most recently
from
45b441a
to
94c1b97
Compare
ciampo
requested review from
youknowriad,
jsnajdr,
jasmussen,
tyxla and
aaronrobertshaw
ciampo
force-pushed
the
feat/toggle-group-control-framer-motion-shared-layout
branch
from
fb1b6a4
to
eac6b2e
Compare
|
Flaky tests detected in e69993e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6087494585
|
jasmussen
approved these changes
Jun 9, 2023
There was a problem hiding this comment.
The user experience is great. Before:
Notice here that the backdrop behind the font size toggle I click actually animates in from nothing/from the left, when the initial control is unset.
After:
Notice here how when the control is initiallly unset, the backdrop just fades in, instead of also animating in. This is an improvement in this PR, nice.
The animation is also slightly slower, that's fine.
jsnajdr
reviewed
Jun 9, 2023
| export { | ||
| __unstableMotion, | ||
| __unstableAnimatePresence, | ||
| __unstableMotionContext, |
There was a problem hiding this comment.
Let's export all the providers (style, components, toolbar context, motion) consistently the same way, as private APIs.
ciampo
force-pushed
the
feat/toggle-group-control-framer-motion-shared-layout
branch
from
8c3c09e
to
e6754b2
Compare
There was a problem hiding this comment.
Nice work here @ciampo, the updated animation is great ✨
Everything is testing as advertised for me.
I couldn't break it in regards to the ToolsPanelItem registration/deregistration. The ToolsPanel stories function as before and the component's unit tests are passing.
The ToggleGroupControl animations were nice and smooth in all the contexts I tested.
I've run out of time today to give a complete code review however I can do that later this week if nobody beats me to it in the meantime.
ciampo
force-pushed
the
feat/toggle-group-control-framer-motion-shared-layout
branch
3 times, most recently
from
320a39f
to
3619e38
Compare
I've looked a bit more into this, and effectively it looks like my previous changes introduced a regression. I've spent some time looking into it, and the way I managed to restore support for both the uncontrolled and controlled usage of the component introduces a few changes:
A few considerations / questions: @diegohaz : would you mind checking the way I've implemented controlled/uncontrolled support around
If we're ok with these changes but you find the PR's scope too large, I'm also happy to split the "controlled / uncontrolled" refactor to a separate PR, to be merged before this one. |
ciampo
force-pushed
the
feat/toggle-group-control-framer-motion-shared-layout
branch
from
5c66be2
to
2364d02
Compare
ciampo
deleted the
feat/toggle-group-control-framer-motion-shared-layout
branch
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.
What?
Rewrite
ToggleGroupControls backdrop animation usingframer-motion's shared layout animations.This PR supersedes #49822, since in the meantime:
framer-motiongot updated to a more recent version (Update the framer motion dependency to the latest version #49822)ToggleGroupControlwent through some major internal refactoring (ToggleGroupControl: Add de-selectable variant #45123 in particular)Why?
The current implementation for the backdrop animation is complex, difficult to read, imperative (based off arbitrary timeouts), and buggy.
How?
Main changes are:
ToggleGroupControlBackdrop, and the imperative logic that came with itframer-motion's shared layout animations:layoutIdattribute to tag the components that participate to the same shared layout animationToggleGroupControlalso defines its own "namespace" for the shared layout animations via theLayoutGroupcomponent, in order to avoid conflicts between different instances of the componentInspectorControl'sSlotandFillcomponents to forward theMotionContextusePrevious+useUpdateEffect. The refactor meant:useControlledValuefor the "as button" version of the componentreakittoariakitfor the "as radio" version of the component (since it has support for controlled/uncontrolled logic)undefinedvalue is passed and based on the previous value of thevalueprop (to maintain backwards-compat)Testing Instructions
ToggleGroupControlcomponent and make sure that the backdrop animates as expectedToggleGroupControl) works and animates as expectedToggleGroupControl) works and animates as expectedToggleGroupControl) works and animates as expectedTesting Instructions for Keyboard
Screenshots or screencast
Kapture.2023-06-08.at.21.01.31.mp4