-
Notifications
You must be signed in to change notification settings - Fork 729
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
feat(tooltip): Enable custom portal container for tooltip #1567
base: master
Are you sure you want to change the base?
Changes from all commits
ac66f31
ae56a47
3fbf8a9
7a5e3b3
99066ae
6aeb835
0ec63f5
ec94a27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,13 @@ | ||
import React, { useMemo } from 'react'; | ||
import React, { useCallback, useMemo, useState } from 'react'; | ||
import useMeasure, { RectReadOnly, Options as BaseUseMeasureOptions } from 'react-use-measure'; | ||
import useResizeObserver from '@react-hook/resize-observer'; | ||
|
||
import Portal, { PortalProps } from '../Portal'; | ||
import Tooltip, { TooltipProps } from '../tooltips/Tooltip'; | ||
import TooltipWithBounds from '../tooltips/TooltipWithBounds'; | ||
|
||
export type TooltipInPortalProps = TooltipProps & | ||
Pick<UseTooltipPortalOptions, 'detectBounds' | 'zIndex'>; | ||
Pick<UseTooltipPortalOptions, 'detectBounds' | 'portalContainer' | 'zIndex'>; | ||
|
||
export type UseTooltipInPortal = { | ||
containerRef: (element: HTMLElement | SVGElement | null) => void; | ||
|
@@ -24,6 +25,8 @@ export type UseTooltipPortalOptions = Pick<PortalProps, 'zIndex'> & { | |
scroll?: boolean; | ||
/** You can optionally inject a ResizeObserver polyfill. */ | ||
polyfill?: BaseUseMeasureOptions['polyfill']; | ||
/** Optional container for the portal. */ | ||
portalContainer?: HTMLDivElement; | ||
}; | ||
|
||
/** | ||
|
@@ -32,16 +35,34 @@ export type UseTooltipPortalOptions = Pick<PortalProps, 'zIndex'> & { | |
*/ | ||
export default function useTooltipInPortal({ | ||
detectBounds: detectBoundsOption = true, | ||
portalContainer, | ||
zIndex: zIndexOption, | ||
...useMeasureOptions | ||
}: UseTooltipPortalOptions | undefined = {}): UseTooltipInPortal { | ||
const [containerRef, containerBounds, forceRefreshBounds] = useMeasure(useMeasureOptions); | ||
|
||
const [portalContainerRect, setPortalContainerRect] = useState<DOMRect | null>( | ||
portalContainer?.getBoundingClientRect() ?? null, | ||
); | ||
|
||
const updatePortalContainerRect = useCallback(() => { | ||
if (portalContainer) { | ||
setPortalContainerRect(portalContainer?.getBoundingClientRect()); | ||
} | ||
}, [portalContainer]); | ||
|
||
React.useEffect(updatePortalContainerRect, [ | ||
containerBounds, | ||
portalContainer, | ||
updatePortalContainerRect, | ||
]); | ||
useResizeObserver(portalContainer ?? null, updatePortalContainerRect); | ||
|
||
const TooltipInPortal = useMemo( | ||
() => | ||
function ({ | ||
left: containerLeft = 0, | ||
top: containerTop = 0, | ||
left: tooltipLeft = 0, | ||
top: tooltipTop = 0, | ||
detectBounds: detectBoundsProp, // allow override at component-level | ||
zIndex: zIndexProp, // allow override at the component-level | ||
...tooltipProps | ||
|
@@ -50,16 +71,41 @@ export default function useTooltipInPortal({ | |
const zIndex = zIndexProp == null ? zIndexOption : zIndexProp; | ||
const TooltipComponent = detectBounds ? TooltipWithBounds : Tooltip; | ||
// convert container coordinates to page coordinates | ||
const portalLeft = containerLeft + (containerBounds.left || 0) + window.scrollX; | ||
const portalTop = containerTop + (containerBounds.top || 0) + window.scrollY; | ||
const portalLeft = portalContainer | ||
? tooltipLeft - (portalContainerRect?.left || 0) + (containerBounds.left || 0) | ||
: tooltipLeft + (containerBounds.left || 0) + window.scrollX; | ||
const portalTop = portalContainer | ||
? tooltipTop - (portalContainerRect?.top || 0) + (containerBounds.top || 0) | ||
: tooltipTop + (containerBounds.top || 0) + window.scrollY; | ||
|
||
const additionalTooltipProps = | ||
detectBounds && portalContainer | ||
? { | ||
portalContainerPosition: { | ||
left: portalContainerRect?.left || 0, | ||
top: portalContainerRect?.top || 0, | ||
}, | ||
visualParentRect: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think this should always be passed as props even without a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say yes ideally, for consistency and because it would fix the bound detection for the case of tooltips in the default portal. That being said, since we're dealing with a library here and the bound detection has been broken since the beginning when rendering the tooltip in the default portal, I'm worried that people may have worked under the assumption that it would stay that way, and that fixing this bound detection retroactively may break the tooltip position in their respective apps. That's the reason why I only applied this prop to the case of a custom portal container. But if you feel confident that we could do this without inadvertently breaking other users' tooltips, I'd be down for doing it, just let me know! |
||
width: containerBounds.width, | ||
height: containerBounds.height, | ||
left: containerBounds.left, | ||
top: containerBounds.top, | ||
}, | ||
} | ||
: {}; | ||
|
||
return ( | ||
<Portal zIndex={zIndex}> | ||
<TooltipComponent left={portalLeft} top={portalTop} {...tooltipProps} /> | ||
<Portal container={portalContainer} zIndex={zIndex}> | ||
<TooltipComponent | ||
left={portalLeft} | ||
top={portalTop} | ||
{...tooltipProps} | ||
{...additionalTooltipProps} | ||
/> | ||
</Portal> | ||
); | ||
}, | ||
[detectBoundsOption, zIndexOption, containerBounds.left, containerBounds.top], | ||
[containerBounds, detectBoundsOption, portalContainer, portalContainerRect, zIndexOption], | ||
); | ||
|
||
return { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,20 @@ import { TooltipPositionProvider } from '../context/TooltipPositionContext'; | |
|
||
export type TooltipWithBoundsProps = TooltipProps & | ||
React.HTMLAttributes<HTMLDivElement> & | ||
WithBoundingRectsProps & { nodeRef?: React.Ref<HTMLDivElement> }; | ||
WithBoundingRectsProps & { | ||
nodeRef?: React.Ref<HTMLDivElement>; | ||
/** | ||
* When the tooltip is in a portal, this is the position of the portal | ||
* container to be used for offsetting calculations around bounds as a consequence. | ||
*/ | ||
portalContainerPosition?: { left: number; top: number }; | ||
/** | ||
* When the tooltip is in a portal, the portal container becomes the direct parent of the tooltip. | ||
* Often the portal is not what we want the tooltip to be bound to. Another visual parent can be specified | ||
* by specifying its dimensions here. | ||
*/ | ||
visualParentRect?: { width: number; height: number; left: number; top: number }; | ||
Comment on lines
+11
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I did not change the behavior for the case with no custom portal container for backward compatibility concerns, but I did get it to work as expected in the case when a custom portal container is provided and when the bounds options are on. I think this will help my team fix a bug where the bottom of our tooltip gets cropped sometimes. Fixing it was not super intuitive at first, I hope my comments and the variable names make sufficient sense to understand what's going on here. |
||
}; | ||
|
||
function TooltipWithBounds({ | ||
children, | ||
|
@@ -20,19 +33,26 @@ function TooltipWithBounds({ | |
top: initialTop = 0, | ||
unstyled = false, | ||
nodeRef, | ||
portalContainerPosition, | ||
visualParentRect: visualParentBounds = parentBounds, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than introducing a new prop (this component is getting super complex), could There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did try to overwrite the |
||
...otherProps | ||
}: TooltipWithBoundsProps) { | ||
let transform: React.CSSProperties['transform']; | ||
let placeTooltipLeft = false; | ||
let placeTooltipUp = false; | ||
|
||
if (ownBounds && parentBounds) { | ||
if (ownBounds && visualParentBounds) { | ||
let left = initialLeft; | ||
let top = initialTop; | ||
|
||
if (parentBounds.width) { | ||
const rightPlacementClippedPx = left + offsetLeft + ownBounds.width - parentBounds.width; | ||
const leftPlacementClippedPx = ownBounds.width - left - offsetLeft; | ||
if (visualParentBounds.width) { | ||
const leftInVisualParent = | ||
left + | ||
(portalContainerPosition?.left || 0) - | ||
(portalContainerPosition ? visualParentBounds.left : 0); | ||
const rightPlacementClippedPx = | ||
leftInVisualParent + offsetLeft + ownBounds.width - visualParentBounds.width; | ||
const leftPlacementClippedPx = ownBounds.width - leftInVisualParent - offsetLeft; | ||
placeTooltipLeft = | ||
rightPlacementClippedPx > 0 && rightPlacementClippedPx > leftPlacementClippedPx; | ||
} else { | ||
|
@@ -42,9 +62,14 @@ function TooltipWithBounds({ | |
rightPlacementClippedPx > 0 && rightPlacementClippedPx > leftPlacementClippedPx; | ||
} | ||
|
||
if (parentBounds.height) { | ||
const bottomPlacementClippedPx = top + offsetTop + ownBounds.height - parentBounds.height; | ||
const topPlacementClippedPx = ownBounds.height - top - offsetTop; | ||
if (visualParentBounds.height) { | ||
const topInVisualParent = | ||
top + | ||
(portalContainerPosition?.top || 0) - | ||
(portalContainerPosition ? visualParentBounds.top : 0); | ||
const bottomPlacementClippedPx = | ||
topInVisualParent + offsetTop + ownBounds.height - visualParentBounds.height; | ||
const topPlacementClippedPx = ownBounds.height - topInVisualParent - offsetTop; | ||
placeTooltipUp = | ||
bottomPlacementClippedPx > 0 && bottomPlacementClippedPx > topPlacementClippedPx; | ||
} else { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
window.scrollX/Y
not relevant for these?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not indeed. I don't know enough about the internals of React portals to explain exactly why, but in the case when a custom portal container is used, including
window.scrollX/Y
in the calculation of the tooltip position leads to bugs like this:Screen.Recording.2022-11-10.at.2.28.47.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah maybe when using a custom container, the window x/y is already accounted for. thanks for the video!