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

feat(tooltip): Enable custom portal container for tooltip #1567

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Prettier + Readme update
  • Loading branch information
Florent Bouquet committed Jan 19, 2023
commit ae56a476b5b122269ac1864a3e505c456541c210
35 changes: 18 additions & 17 deletions packages/visx-demo/src/sandboxes/visx-tooltip/Example.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,18 @@ const tooltipStyles = {

export default function Example({ width, height, showControls = true }: TooltipProps) {
const [tooltipShouldDetectBounds, setTooltipShouldDetectBounds] = useState(true);
const [tooltipPortalShouldUseCustomContainer, setTooltipPortalShouldUseCustomContainer] = useState(true);
const [tooltipPortalShouldUseCustomContainer, setTooltipPortalShouldUseCustomContainer] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

simplifying a bit

Suggested change
const [tooltipPortalShouldUseCustomContainer, setTooltipPortalShouldUseCustomContainer] =
const [tooltipShouldUseCustomContainer, setTooltipShouldUseCustomContainer] =

useState(true);
const [renderTooltipInPortal, setRenderTooltipInPortal] = useState(false);
const overlayRootRef = React.useRef<HTMLDivElement | null>(null);

const { containerRef, containerBounds, TooltipInPortal } = useTooltipInPortal(
{
scroll: true,
detectBounds: tooltipShouldDetectBounds,
portalContainer: tooltipPortalShouldUseCustomContainer ? overlayRootRef.current ?? undefined : undefined,
}
);
const { containerRef, containerBounds, TooltipInPortal } = useTooltipInPortal({
scroll: true,
detectBounds: tooltipShouldDetectBounds,
portalContainer: tooltipPortalShouldUseCustomContainer
? overlayRootRef.current ?? undefined
: undefined,
});

const {
showTooltip,
Expand Down Expand Up @@ -91,7 +92,7 @@ export default function Example({ width, height, showControls = true }: TooltipP
<OverlayLayer
className="overlay-layer overlay-under-tooltip"
container={overlayRootRef.current}
text='We want this to appear under the tooltip.'
text="We want this to appear under the tooltip."
/>
{tooltipOpen ? (
<>
Expand Down Expand Up @@ -125,15 +126,13 @@ export default function Example({ width, height, showControls = true }: TooltipP
</TooltipComponent>
</>
) : (
<div className="no-tooltip">
Move or touch the canvas to see the tooltip
</div>
<div className="no-tooltip">Move or touch the canvas to see the tooltip</div>
)}
<OverlayLayer
className="overlay-layer overlay-over-tooltip"
container={overlayRootRef.current}
placeAfterTooltipInDom // Force DOM node to be placed after tooltip for demo purposes
text='We want this to appear over the tooltip.'
text="We want this to appear over the tooltip."
/>
<div className="z-index-bummer">
I have an annoying z-index. Try&nbsp;
Expand Down Expand Up @@ -171,7 +170,9 @@ export default function Example({ width, height, showControls = true }: TooltipP
<input
type="checkbox"
checked={tooltipPortalShouldUseCustomContainer}
onChange={() => setTooltipPortalShouldUseCustomContainer(!tooltipPortalShouldUseCustomContainer)}
onChange={() =>
setTooltipPortalShouldUseCustomContainer(!tooltipPortalShouldUseCustomContainer)
}
/>
&nbsp;Tooltip portal in custom container
</label>
Expand Down Expand Up @@ -274,15 +275,15 @@ const OverlayLayer = function OverlayLayer({
text,
}: OverlayLayerProps) {
if (container) {
// Since we re-render the tooltip every time the pointer moves and its DOM node
// Since we re-render the tooltip every time the pointer moves and its DOM node
// is placed at the end of the container, if placeAfterTooltipInDom is true we
// also want to re-render the overlay layer
const key = placeAfterTooltipInDom ? Math.random() : "overlay-under";
const key = placeAfterTooltipInDom ? Math.random() : 'overlay-under';
return ReactDOM.createPortal(
<div className={className} key={key}>
{text}
</div>,
container
container,
);
} else {
return null;
Expand Down
5 changes: 4 additions & 1 deletion packages/visx-tooltip/Readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ object or inject it cleanly using the `polyfill` config option below.

`useTooltipInPortal` is a hook which gives you a `TooltipInPortal` component for rendering `Tooltip`
or `TooltipWithBounds` in a `Portal`, outside of your component DOM tree which can be useful in many
circumstances (see below for more on `Portal`s).
circumstances (see below for more on `Portal`s). A custom portal container can be provided
via the `portalContainer` config option.

##### API

Expand All @@ -128,6 +129,8 @@ type Options = {
scroll?: boolean
/** You can optionally inject a resize-observer polyfill */
polyfill?: { new (cb: ResizeObserverCallback): ResizeObserver }
/** Optional container for the portal. */
portalContainer?: HTMLDivElement;
/** Optional z-index to set on the Portal div */
zIndex?: number | string;
}
Expand Down
18 changes: 15 additions & 3 deletions packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ export default function useTooltipInPortal({
...tooltipProps
}: TooltipInPortalProps) {
const detectBounds = detectBoundsProp == null ? detectBoundsOption : detectBoundsProp;
const portalContainer = portalContainerProp == null ? portalContainerOption : portalContainerProp;
const portalContainer =
portalContainerProp == null ? portalContainerOption : portalContainerProp;
const portalContainerRect = portalContainer?.getBoundingClientRect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be quite expensive to call each render which is why we opted to use useMeasure for the container.

Is it necessary to support the portalContainerProp override or can it be passed in as an option? If the latter is fine we could use useMeasure for the portalContainer as well. that's tricky though because it may not always update when an element resize happens etc. (which is what forceRefreshBounds is for)

if this simplifies things we could leave it for now.

Copy link
Author

Choose a reason for hiding this comment

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

Oh that's a good catch. 🤔 I don't think we necessarily need to support the override at the component level since typically a portal container doesn't move in the screen. I'll drop this prop at the tooltip level and just keep it on useTooltipInPortal.

However, I'll keep passing in the HTMLDivElement directly to useTooltipInPortal and memoize the bounding rect instead of using useMeasure for this if that's OK, because having to deal with the old fashioned ref useMeasure returns makes things harder on the consumer side in my opinion.

const zIndex = zIndexProp == null ? zIndexOption : zIndexProp;
const TooltipComponent = detectBounds ? TooltipWithBounds : Tooltip;
Expand All @@ -67,11 +68,22 @@ export default function useTooltipInPortal({

return (
<Portal container={portalContainer} zIndex={zIndex}>
<TooltipComponent left={portalLeft} top={portalTop} {...tooltipProps} {...additionalTooltipProps} />
<TooltipComponent
left={portalLeft}
top={portalTop}
{...tooltipProps}
{...additionalTooltipProps}
/>
</Portal>
);
},
[detectBoundsOption, portalContainerOption, zIndexOption, containerBounds.left, containerBounds.top],
[
detectBoundsOption,
portalContainerOption,
zIndexOption,
containerBounds.left,
containerBounds.top,
],
);

return {
Expand Down
27 changes: 14 additions & 13 deletions packages/visx-tooltip/test/Portal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,19 @@ import { Portal } from '../src';
const PortalWithContainer = () => {
// useState rather than useRef so it will react to the ref being available for testing purposes
const [portalContainer, setPortalContainer] = React.useState<HTMLDivElement | null>(null);
const onRefChange = React.useCallback(
(node) => {
setPortalContainer(node);
},
[]
);
const onRefChange = React.useCallback((node) => {
setPortalContainer(node);
}, []);
return (
<>
<div data-testid='inner-div' ref={onRefChange} />
<div data-testid="inner-div" ref={onRefChange} />
{portalContainer && (
<Portal container={portalContainer}>
<div data-testid='element-in-portal'></div>
<div data-testid="element-in-portal"></div>
</Portal>
)}
</>
)
);
};

describe('Portal', () => {
Expand All @@ -32,21 +29,25 @@ describe('Portal', () => {
it('should render children at the document root', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for adding such great tests! 🙌

render(
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

would this make more sense as a test setup since you wouldn't expect to find element-in-portal inside inner-div if it's a sibling?

Suggested change
<>
<div data-testid="inner-div">
<Portal>
<div data-testid="element-in-portal" />
</Portal>
</div>

Copy link
Author

Choose a reason for hiding this comment

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

That's a great point!

<div data-testid='inner-div' />
<div data-testid="inner-div" />
<Portal>
<div data-testid='element-in-portal'></div>
<div data-testid="element-in-portal"></div>
</Portal>
</>,
);
const elementInPortal = await screen.findByTestId('element-in-portal');
expect(elementInPortal).toBeInTheDocument();
expect(within(screen.getByTestId('inner-div')).queryByTestId('element-in-portal')).not.toBeInTheDocument();
expect(
within(screen.getByTestId('inner-div')).queryByTestId('element-in-portal'),
).not.toBeInTheDocument();
});

it('should render children in the provided portal container', async () => {
render(<PortalWithContainer />);
const elementInPortal = await screen.findByTestId('element-in-portal');
expect(elementInPortal).toBeInTheDocument();
expect(within(screen.getByTestId('inner-div')).getByTestId('element-in-portal')).toBeInTheDocument();
expect(
within(screen.getByTestId('inner-div')).getByTestId('element-in-portal'),
).toBeInTheDocument();
});
});
39 changes: 20 additions & 19 deletions packages/visx-tooltip/test/useTooltipInPortal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,28 @@ interface TooltipWithPortalContainerProps {
shouldUsePortalContainer: boolean;
}

const TooltipWithPortalContainer = ({ shouldUsePortalContainer }: TooltipWithPortalContainerProps) => {
const TooltipWithPortalContainer = ({
shouldUsePortalContainer,
}: TooltipWithPortalContainerProps) => {
// useState rather than useRef so it will react to the ref being available for testing purposes
const [portalContainer, setPortalContainer] = React.useState<HTMLDivElement | null>(null);
const onRefChange = React.useCallback(
(node) => {
setPortalContainer(node);
},
[]
);
const onRefChange = React.useCallback((node) => {
setPortalContainer(node);
}, []);

const { TooltipInPortal } = useTooltipInPortal(
{
polyfill: ResizeObserver,
portalContainer: shouldUsePortalContainer ? portalContainer ?? undefined : undefined,
}
);
const { TooltipInPortal } = useTooltipInPortal({
polyfill: ResizeObserver,
portalContainer: shouldUsePortalContainer ? portalContainer ?? undefined : undefined,
});

return (
<>
<div data-testid='inner-div' ref={shouldUsePortalContainer ? onRefChange : undefined} />
<div data-testid="inner-div" ref={shouldUsePortalContainer ? onRefChange : undefined} />
<TooltipInPortal>
<div data-testid='element-in-tooltip'></div>
<div data-testid="element-in-tooltip"></div>
</TooltipInPortal>
</>
)
);
};

describe('useTooltipInPortal()', () => {
Expand All @@ -63,7 +60,7 @@ describe('useTooltipInPortal()', () => {
const zIndex = wrapper.find('Portal').prop('zIndex');
expect(zIndex).toBe(1);
});

it('should pass zIndex prop from component to Portal', () => {
const wrapper = shallow(
<TooltipWithZIndex zIndexOption={1} zIndexProp="var(--tooltip-zindex)" />,
Expand All @@ -81,14 +78,18 @@ describe('useTooltipInPortal()', () => {
render(<TooltipWithPortalContainer shouldUsePortalContainer={false} />);
const elementInPortal = await screen.findByTestId('element-in-tooltip');
expect(elementInPortal).toBeInTheDocument();
expect(within(screen.getByTestId('inner-div')).queryByTestId('element-in-tooltip')).not.toBeInTheDocument();
expect(
within(screen.getByTestId('inner-div')).queryByTestId('element-in-tooltip'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess same question on this test since element-in-tooltip is a sibling / wouldn't be expected to be a child of inner-div

).not.toBeInTheDocument();
});

it('it should render tooltip in the provided portal container', async () => {
render(<TooltipWithPortalContainer shouldUsePortalContainer={true} />);
const elementInPortal = await screen.findByTestId('element-in-tooltip');
expect(elementInPortal).toBeInTheDocument();
expect(within(screen.getByTestId('inner-div')).getByTestId('element-in-tooltip')).toBeInTheDocument();
expect(
within(screen.getByTestId('inner-div')).getByTestId('element-in-tooltip'),
).toBeInTheDocument();
});
});
});