Popover: fix and improve opening animation, use framer motion #43186
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
This was referenced Aug 12, 2022
|
Size Change: -88 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Bug
ciampo
changed the title
ciampo
force-pushed
the
fix/popover-opening-animations
branch
from
d4ee91e
to
c4bd972
Compare
ciampo
requested review from
talldan,
jasmussen,
mirka,
ntsekouras,
aaronrobertshaw,
andrewserong and
chad1008
|
Really nice work! Works for me — toolbar popovers open down and to the right, but the centered-below-control-point gradient popover opens down and center: Thanks for this fix! 🚢 |
talldan
reviewed
Aug 15, 2022
There was a problem hiding this comment.
Looks really good. Code seems solid.
I noticed a potential issue—in the testing environment (localhost:8889 when using wp-env), the 'Gutenberg Test Plugin, Disables the CSS animations' plugin is used to disable animations. It makes puppeteer end to end tests more stable, since that library doesn't do auto-waiting like playwright does.
In this PR popover animations still seem to be active.
Good find! I'll have a look and report back (also curious to see if this issue affects other |
|
As I suspected, all @talldan , we could treat that as a separate issue and move ahead with this PR — what do you think? |
ciampo
force-pushed
the
fix/popover-opening-animations
branch
2 times, most recently
from
77ca5a1
to
710dd04
Compare
…clarity around poopover positioning
ciampo
force-pushed
the
fix/popover-opening-animations
branch
from
f892602
to
65d458f
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.
What?
Tracked in #42770
After the regression introduced by #40740, this PR aims at restoring (and improving) the
Popover's opening animationWhy?
#40740 introduces a few big changes to
Popover— in particular, it introduced a newplacementprop (that should replace the legacypositionprop). Alongside theplacementprop, #40470 introduced two new functions:positionToPlacement: a function to "convert"positionvalues toplacementvaluesplacementToAnimationOrigin: a function to associate an animation origin, given theplacementThis refactor caused the popover to animate unexpectedly (i.e. from the from animation origin (example).
Furthermore, as I looked into solving this issue, I noticed that
Popoverstill relied on theanimatecomponent, which was supposed to be deprecated already some time ago (see #27390).Finally, I also noticed that the
Popoverwas animating even when in its expanded mode, which is supposed to occupy the full screen on mobile — I believe that animating a piece of UI as big as the whole screen with a translate/scale animation could be excessive.How?
framer-motion(instead of theanimatecomponent) for thePopover's animationanimatecomponent:0.1scubic-bezier(0, 0, 0.2, 1)xandyaxis)0to1opacity)animateproperty, the popover won't animate when in its expanded state (on mobile). This change was caused by two reasons:placement— even if the placement isn't really taken into account in the expanded state (the popover is full screen)placementand/or is less overwhelming visually.placementToAnimationOriginlogic, using a lookup table instead of hard-to-read logic for the animation originpositionToPlacementandplacementToAnimationOriginto a separateutils.jsfilepositionToPlacementfunction and the variable parts of theplacementToAnimationOriginfunctionTesting Instructions
For ease of debug, the animation duration can be changes like so:
Storybook
Open Storybook and play around with the
Popoverexamples. In particular:placementvalue, the popover animates from the correct animation originanimateprop is set tofalseexpandOnMobileprop is set totrueand the popover is rendered in mobile viewports (even if theanimateprop is set totrue)prefers-reduced-motionflag is set by the user (even if theanimateprop is set totrue)In the editor
Do a sanity check across the editor and make sure that the popover behaves (and animates when necessary) as expected
Known issues
#42770 has a list of known issues.
Specifically to testing for animation origin, it is possible that the
positionToPlacementfunction introduced in #40740 is not converting the position 100% correctly. For the sake of this PR's scope, we should focus on theplacementto animation origin logic (thepositiontoplacementlogic can be tackled separately).Screenshots or screencast
Storybook (normal speed), this PR:
popover-animation.mp4
Storybook (slower speed), this PR:
popover-animation.mp4
Editor (normal speed),
trunk:popover-animation-editor-trunk.mp4
Editor (normal speed), this PR:
popover-animation-editor-pr.mp4