Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use BoundsObserver to keep tooltip over the wrapper #18782

Merged
merged 17 commits into from
May 19, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 100 additions & 13 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"@react-navigation/drawer": "github:Expensify/react-navigation#react-navigation-drawer-v6.5.0-alpha1-gitpkg",
"@react-navigation/native": "6.0.13",
"@react-navigation/stack": "6.3.1",
"@react-ng/bounds-observer": "^0.2.1",
"@ua/react-native-airship": "^15.2.3",
"awesome-phonenumber": "^5.4.0",
"babel-plugin-transform-remove-console": "^6.9.4",
Expand Down
49 changes: 25 additions & 24 deletions src/components/Tooltip/TooltipRenderedOnPageBody.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ const propTypes = {
/** The distance between the top of the wrapper view and the top of the window */
yOffset: PropTypes.number.isRequired,

/** The width of the tooltip wrapper */
wrapperWidth: PropTypes.number.isRequired,
/** The width of the tooltip's target wrapper */
targetWrapperWidth: PropTypes.number.isRequired,

/** The Height of the tooltip wrapper */
wrapperHeight: PropTypes.number.isRequired,
/** The height of the tooltip's target wrapper */
targetWrapperHeight: PropTypes.number.isRequired,

/** Any additional amount to manually adjust the horizontal position of the tooltip.
A positive value shifts the tooltip to the right, and a negative value shifts it to the left. */
Expand Down Expand Up @@ -63,9 +63,9 @@ const TooltipRenderedOnPageBody = (props) => {
// The width of tooltip's inner content. Has to be undefined in the beginning
// as a width of 0 will cause the content to be rendered of a width of 0,
// which prevents us from measuring it correctly.
const [tooltipContentWidth, setTooltipContentWidth] = useState(undefined);
const [tooltipWidth, setTooltipWidth] = useState(0);
const [tooltipHeight, setTooltipHeight] = useState(0);
const [contentMeasuredWidth, setContentMeasuredWidth] = useState(undefined);
const [wrapperMeasuredWidth, setWrapperMeasuredWidth] = useState(0);
const [wrapperMeasuredHeight, setWrapperMeasuredHeight] = useState(0);
const contentRef = useRef();
const wrapper = useRef();

Expand All @@ -81,38 +81,39 @@ const TooltipRenderedOnPageBody = (props) => {
// because of the late update of the width and the height from onLayout.
const rect = wrapper.current.getBoundingClientRect();

setTooltipWidth(rect.width);
setTooltipHeight(rect.height);
setTooltipContentWidth(contentRef.current.offsetWidth);
setWrapperMeasuredWidth(rect.width);
setWrapperMeasuredHeight(rect.height);
setContentMeasuredWidth(contentRef.current.offsetWidth);
}, []);

const {animationStyle, tooltipWrapperStyle, tooltipTextStyle, pointerWrapperStyle, pointerStyle} = useMemo(
const {animationStyle, wrapperStyle, textStyle, pointerWrapperStyle, pointerStyle} = useMemo(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We named wrapperStyle to tooltipwrapperStyle to distinguish it from pointerWrapperStyle. IMO, that was fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the reference is named wrapper 🙁 I renamed wrapper to rootWrapper and wrapperStyle to rootWrapperStyle, trying to address both concerns

() =>
getTooltipStyles(
props.animation,
props.windowWidth,
props.xOffset,
props.yOffset,
props.wrapperWidth,
props.wrapperHeight,
props.targetWrapperWidth,
props.targetWrapperHeight,
props.maxWidth,
tooltipWidth,
tooltipHeight,
tooltipContentWidth,
wrapperMeasuredWidth,
wrapperMeasuredHeight,
contentMeasuredWidth,
props.shiftHorizontal,
props.shiftVertical,
wrapper.current,
),
[
props.animation,
props.windowWidth,
props.xOffset,
props.yOffset,
props.wrapperWidth,
props.wrapperHeight,
props.targetWrapperWidth,
props.targetWrapperHeight,
props.maxWidth,
tooltipWidth,
tooltipHeight,
tooltipContentWidth,
wrapperMeasuredWidth,
wrapperMeasuredHeight,
contentMeasuredWidth,
props.shiftHorizontal,
props.shiftVertical,
],
Expand All @@ -125,10 +126,10 @@ const TooltipRenderedOnPageBody = (props) => {
content = (
<Text
numberOfLines={props.numberOfLines}
style={tooltipTextStyle}
style={textStyle}
>
<Text
style={tooltipTextStyle}
style={textStyle}
ref={contentRef}
>
{props.text}
Expand All @@ -140,7 +141,7 @@ const TooltipRenderedOnPageBody = (props) => {
return ReactDOM.createPortal(
<Animated.View
ref={wrapper}
style={[tooltipWrapperStyle, animationStyle]}
style={[wrapperStyle, animationStyle]}
>
{content}
<View style={pointerWrapperStyle}>
Expand Down
Loading