Popover: pass missing anchor ref to the getAnchorRect callback prop #42076
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
ciampo
commented
Jun 30, 2022
| @@ -272,10 +272,10 @@ const Popover = ( | |||
| return anchorRect; | |||
| }, | |||
| }; | |||
| } else if ( getAnchorRect ) { | |||
| } else if ( getAnchorRect && anchorRefFallback.current ) { | |||
There was a problem hiding this comment.
This extra check is to mimic the logic from the early return statement that existed prior to the refactor from #40740).
Not 100% sure about introducing it though, as it may cause regressions (since the getAnchorRect callback may be called fewer times, in case anchorRefFallback.current is not defined).
There was a problem hiding this comment.
Oh, interesting question. In this case from reading through the series of if / else statements it sounds like if this if block is reached, then the output should include the <span ref={ anchorRefFallback } so it'd be expected by this point that anchorRefFallback.current would most likely be truthy?
Given that the README says that the callback will be called with a reference to the popover anchor element, I think adding this extra check sounds consistent with the expectations of the component.
We might just want to double-check that the BlockPopover and ListViewDropIndicator components are working as expected — which is slightly tricky because they're already components that behave slightly awkwardly sometimes 😅
But overall, the logic of re-introducing this check sounds good to me.
ciampo
requested review from
mirka,
aaronrobertshaw,
andrewserong,
chad1008,
ntsekouras and
jorgefilipecosta
ciampo
added
[Type] Bug
|
Size Change: +414 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Jul 1, 2022
There was a problem hiding this comment.
Thanks for digging into this one @ciampo! In testing this doesn't appear to cause any differences of behaviour in the block popover or list view drop indicator components 👍
While logging out in the Storybook example, I noticed that getAnchorRect() is also called on this line:
undefined when it is first called, but by the second call (the line change in this PR), my callback receives the correct element:
I suppose this potentially raises a couple of questions:
- Should that call be changed to also attempt to pass in the fallback ref? Or,
- Is it okay that the callback will be called sometimes with an
undefinedvalue, in which case, perhaps we don't need the additional check for.current?
For the former question, the call to getAnchorRect() appears to assume that the response from that function will be successful even if it's called with undefined, which could be unexpected for consumers of this component 🤔
I'll be AFK on Monday but happy to do more testing next week if this hasn't landed already 🙂
andrewserong
changed the title
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
|
Hey @andrewserong , thank you for your review! Your observations make a lot of sense, and I made changes so that both calls to
Let me know what you think! |
andrewserong
approved these changes
Jul 5, 2022
There was a problem hiding this comment.
Thanks for following up @ciampo! This is still testing well for me in the block editor and storybook, and the changes look good to me, too 🎉
67 tasks
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?
Fix a regression introduced in #40740, by passing the anchor ref as an argument of the
getAnchorRectcallback prop.Why?
Prior to #40740, the
getAnchorRectcallback prop used to receive the fallback anchor ref as an argument. This behavior was removed as part of the refactor in #40740.This PR adds back that behavior.
How?
In an effort to replicate as closely as possible the logic that existed before #40740, the changes that this PR introduces are:
anchorRefFallback.currentref is passed to thegetAnchorRefcallback propgetAnchorRectprop is called only ifanchorRefFallback.currentholds a value (this is to mimic the logic from the early return statement that existed prior to the refactor from Refactor the Popover component to use the floatingUI library #40740)I believe that the reason why this regression hasn't been encountered until now is because it looks like that the few usages of the
getAnchorRectdon't actually rely on its arguments (a quick search across the codebase should confirm this hypothesis).Testing Instructions
getAnchorRefprop to the Storybook example, notice how:undefined