Only refresh the popover if the anchor position changes #11751
Conversation
youknowriad
added
[Type] Bug
youknowriad
force-pushed
the
fix/popover-refresh
branch
from
c9fd7ea to
9d1fb04
Compare
youknowriad
force-pushed
the
fix/popover-refresh
branch
from
1907462 to
7aa6c3a
Compare
mmtr
left a comment
mmtr
left a comment
There was a problem hiding this comment.
It looks good to me. Left a minor note, but I don't consider it a blocker
| if ( didPopoverSizeChange ) { | ||
| this.setState( { popoverSize } ); | ||
| } | ||
| this.anchorRect = anchorRect; |
There was a problem hiding this comment.
What if we also check here if the anchor moves like we do on refreshOnAnchorMove? And then call to this.computePopoverPosition( popoverSize, anchorRect ); only if didPopoverSizeChange or didAnchorRectChange.
There was a problem hiding this comment.
That was the first thing I tried but it's not enough because the anchor don't move when we resize the window and we do need to update the popover in this case.
tofumatt
left a comment
tofumatt
left a comment
There was a problem hiding this comment.
Tested locally and seems good to me!
|
|
||
| // Property used keep track of the previous anchor rect | ||
| // used to compute the popover position and size. | ||
| this.anchorRect = {}; |
There was a problem hiding this comment.
Can't we just say rectangle? We can spare the bytes 😉
| this.anchorRect = {}; | |
| this.anchorRectangle = {}; |
There was a problem hiding this comment.
I guess a bunch of the DOM APIs use Rect, so I suppose for consistency it's fine. I just dislike it, but I guess my beef is with JS, not this variable name 😉
There was a problem hiding this comment.
yes, since this is mainly based on getBoundingClientRect I'd be in favor of keeping "rect" at the moment.
* Only refresh the popover if the anchor position changes (#11751) * Update plugin version to 4.3.0
On small screens where the inserter can't show up fully, this PR #11257 introduced a small regression where the popover was recomputing its height constantly causing this weird behavior.
In This PR, I'm updating it to only recompute if the anchor position changes, we can't recompute if the popover size change because the recomputing itself can cause the popover size to change causing an infinite loop.