Popover: set is-positioned class only after animation has finished #54178
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
jsnajdr
added
[Type] Bug
ciampo
approved these changes
Sep 5, 2023
packages/components/CHANGELOG.md
Outdated
| @@ -8,7 +8,8 @@ | |||
|
|
|||
| ### Enhancements | |||
There was a problem hiding this comment.
Not related to this PR, but I noticed that the Enhancements header is not rendering as expected because (I believe) of a different space character than the normal one. Would you mind fixing that in this PR too? Thank you!
There was a problem hiding this comment.
Good catch, there was a Unicode no-break space character there (C2A0 in UTF-8) 👍
packages/components/CHANGELOG.md
Outdated
| @@ -8,7 +8,8 @@ | |||
|
|
|||
| ### Enhancements | |||
|
|
|||
| - Making Circular Option Picker a `listbox`. Note that while this changes some public API, new props are optional, and currently have default values; this will change in another patch. ([#52255](https://github.com/WordPress/gutenberg/pull/52255)) | |||
| - Making Circular Option Picker a `listbox`. Note that while this changes some public API, new props are optional, and currently have default values; this will change in another patch ([#52255](https://github.com/WordPress/gutenberg/pull/52255)). | |||
| - `Popover`: Add the `is-positioned` CSS class only after the popover has finished animating ([54178](https://github.com/WordPress/gutenberg/pull/54178)). | |||
There was a problem hiding this comment.
A missing # could maybe be the reason why the CI check is failing?
Suggested change
| - `Popover`: Add the `is-positioned` CSS class only after the popover has finished animating ([54178](https://github.com/WordPress/gutenberg/pull/54178)). | |
| - `Popover`: Add the `is-positioned` CSS class only after the popover has finished animating ([#54178](https://github.com/WordPress/gutenberg/pull/54178)). |
|
Thank you for following this up @jsnajdr!! 🙇🏻 |
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 should be the ultimate fix for the flaky popover tests (like
DotTip) that @ramonjd has been trying to fix in #54168. The snapshot differences are caused by the fact that the popover's Framer Motion animation is still running when we are checking the snapshot, and the animated style properties likeopacityortranslateYhave non-deterministic values.The fix is to set the
is-positionedCSS class on the popover element only after the animation is complete. Then the.toBePositionedPopoverJest helper will reliably wait for the animation to finish.To achieve this, I first had to merge the
AnimationWrappercomponent into the baseUnforwardedPopovercomponent. Their internals need to communicate with each other now, and the separation is no longer tenable.After merging, it's easy to introduce a
finishedAnimationstate variable, set by the Framer Motion'sonAnimationCompletecallback, and usefinishedAnimationas another pre-condition foris-positioned.We've discussed this a year ago with @ciampo and @tyxla, but at the time merging the
AnimationWrapperwas too hard. Today, after many incremental refactorings, it's possible.Note that the updated
DotTipsnapshot now correctly reflects the "animation complete" state: opacity is 1, i.e., element is fully visible, and there are notranslateandscaletransforms.