[Popper][Base] Fix Tooltip Anchor Element Setter #35469
Conversation
|
|
@mnajdova @michaldudak this is ready for review when you get a chance! Thanks! |
zannager
added
the
component: Popper
There was a problem hiding this comment.
Nice catch! Changes make sense to me. Here is the CodeSandbox after fix: https://codesandbox.io/s/anchorel-called-with-arg-gcgds2.
I think migration to TypeScript can prevent these type of issues. It's currently being done in #34771.
ZeeshanTamboli
changed the title
|
LGTM @michaldudak Can you take a final look? |
| @@ -84,7 +84,7 @@ const PopperTooltip = React.forwardRef(function PopperTooltip(props, ref) { | |||
| * modifiers.flip is essentially a flip for controlled/uncontrolled behavior | |||
| */ | |||
| const [placement, setPlacement] = React.useState(rtlPlacement); | |||
| const [tooltipAnchorEl, setTooltipAnchorEl] = React.useState(anchorEl); | |||
| const [tooltipAnchorEl, setTooltipAnchorEl] = React.useState(resolveAnchorEl(anchorEl)); | |||
There was a problem hiding this comment.
Since this is always a resolved element, calling it resolvedAnchorEl (or even resolvedAnchorElement, to reduce abbreviations), would make the intents clearer
|
Looks good! I do have one remark about naming, but apart from that it's good to go. |
| @@ -78,13 +78,14 @@ const PopperTooltip = React.forwardRef(function PopperTooltip(props, ref) { | |||
| }, [handlePopperRef]); | |||
| React.useImperativeHandle(popperRefProp, () => popperRef.current, []); | |||
|
|
|||
| const resolvedAnchorElement = resolveAnchorEl(anchorEl); | |||
There was a problem hiding this comment.
@michaldudak Is this what you asked for?
There was a problem hiding this comment.
No, I meant:
-const [tooltipAnchorEl, setTooltipAnchorEl] = React.useState(resolveAnchorEl(anchorEl));
+const [resolvedAnchorElement, setResolvedAnchorElement] = React.useState(resolveAnchorEl(anchorEl));
siriwatknp
force-pushed
the
fix-popper-anchor-el-tooltip-setter
branch
from
863d6d5
to
c7ab9d2
Compare
Problem
When
props.anchorElis a function, it sometimes gets called with one argument when it expects no arguments (see codesandbox example).Cause
This is happening because the state setter
setTooltipAnchorEl(anchorEl)(recently added in #34714) calls a functionanchorElas if it were in the form ofsetTooltipAnchorEl((prev) => next).Solution
We should call
resolveAnchorElonanchorElbefore settingtooltipAnchorEl.