Popover: make more reactive to prop changes, avoid unnecessary updates #43335
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 17, 2022
|
Size Change: +190 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
fix/popover-frame-ref
branch
from
85eb8df
to
4bf6dfc
Compare
ciampo
force-pushed
the
fix/popover-update-deps
branch
from
3054620
to
7c6596d
Compare
ciampo
force-pushed
the
fix/popover-update-deps
branch
from
7c6596d
to
526f465
Compare
ciampo
force-pushed
the
fix/popover-update-deps
branch
from
526f465
to
02c24d6
Compare
ciampo
force-pushed
the
fix/popover-update-deps
branch
3 times, most recently
from
fc5342e
to
71196ee
Compare
ciampo
added
[Type] Bug
ciampo
changed the title
Popover: make more reactive to prop changes, avoid unnecessary updates
ciampo
changed the title
Popover: make more reactive to prop changes, avoid unnecessary updates
ciampo
force-pushed
the
fix/popover-update-deps
branch
3 times, most recently
from
e5379a5
to
c734279
Compare
ciampo
commented
Aug 29, 2022
| @@ -105,3 +105,109 @@ export const getFrameOffset = ( document ) => { | |||
| const iframeRect = frameElement.getBoundingClientRect(); | |||
| return { x: iframeRect.left, y: iframeRect.top }; | |||
| }; | |||
|
|
|||
| export const getReferenceOwnerDocument = ( { | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
For now, I haven't added types or JSDocs because I'm planning on rewriting this component to TypeScript relatively soon (plus, I hope we can get rid of a few permutations soon enough)
ciampo
force-pushed
the
fix/popover-update-deps
branch
from
c734279
to
f6cf77d
Compare
mirka
approved these changes
Aug 30, 2022
There was a problem hiding this comment.
Great work! I found the code to be relatively readable, given the complexity. The changes you described make sense, and I appreciate that you documented all the reasoning in detail.
I did some testing in Storybook/Editor and nothing jumped out to me.
Not sure if this is relevant to the PR, but I did notice this issue in the iframe story when taking these steps (the order matters):
- Set
position="middle left". - Set
noArrow=false. - Arrow position is wrong.
CleanShot.2022-08-30.at.22.03.02.mp4
I'll leave to you to decide if you want more eyes on this PR, or if the issue above should be addressed 👍
| @@ -181,29 +205,6 @@ const Popover = ( | |||
| ? positionToPlacement( position ) | |||
There was a problem hiding this comment.
Not an issue introduced in this PR, but what is supposed to happen when noArrow=false and position="middle center"?
There was a problem hiding this comment.
Good question — I don't think that floating-ui supports an equivalent position.
The plan is to:
- convert all usages of
Popoverto useplacementinstead ofposition - mark
positionas deprecated / non-supported - therefore, avoiding spending time on fixing it unless more reports are filed 😅
ciampo
force-pushed
the
fix/popover-update-deps
branch
from
f6cf77d
to
5f28709
Compare
Well spotted! I took a quick look and the problem doesn't seem to be related to
I'll look more into it and see if:
|
|
Alright, mystery solved! The iframe's offset needs to be manually added to the arrow's position (which is weird, but it's probably a consequence of the other hacks that are being done to offset the popover in the first place — I have the feeling that #42950 could fix this quirk)
I'll merge this PR once CI is ✅ |
ciampo
force-pushed
the
fix/popover-update-deps
branch
2 times, most recently
from
a2d5c82
to
1c0b7c9
Compare
…enceElement to be stored in state
ciampo
force-pushed
the
fix/popover-update-deps
branch
from
1c0b7c9
to
1de9502
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
Rewrite
Popoverinternals (especially effect dependencies and forced updates) to make component more reactive to external change, but also to avoid unnecessary updates.Why?
Looking at
Popoverits code looks quite complex, with many updates fragmented across different hooks with different dependencies. The changes in this PR aim at:How?
An important concept to understand when working on
Popoveris the difference between:Popovercomponent)List of changes:
fallbackReferenceElementand thereferenceOwnerDocument. This is to ensure reactive updates when any of those variables change (as suggested in the docs)referenceOwnerDocumentand of thereferenceelement under the sameuseLayoutEffectcall. This ensures that these two updates happen together. The dependency list of theuseLayoutEffectcall has also been updated to include all possible permutations ofanchorRef. Finally, the code calculating these two values has been moved to a separate file, in order to make the code in thePopovershorter and easier to follow.autoUpdatecall from theuseLayoutEffecthook to thewhileElementsMountedoption of theuseFloatinghook. TheautoUpdatecan be an expensive function to call, and instead of calling it explicitly in theuseLayoutEffecthook, we should letfloating-uidecide when to call it (this follows the suggestion from the docs)arrowmiddleware. The middleware is not added conditionally (as it already does that internally, based on the value of the arrowelement), and the arrow ref is now a callback ref which callsupdate(), thus making sure that the popover's arrow is always positioned correctly. (as explained in the docs)framer-motionto animate the popover towards its new placement, which would throw offfloating-ui's measurements / calculations.Testing Instructions
In Storybook
offset,hasArrow,placement...) and make sure that the popover on screen updates correctlyWithSlotOutsideIframefinally aligns correctly to its anchor inside the iframeIn the Editor
trunkbehaviorScreenshots or screencast
Storybook - default story
trunk(notice how the popover's position is not correct when changingplacement)popover-trunk.mp4
This PR:
popover-pr.mp4
Storybook - iframe story
trunk(notice how the popover is not positioned correctly with respect to its anchor in the iframe)popover-iframe-trunk.mp4
This PR:
popover-iframe-pr.mp4