Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
5 changes: 5 additions & 0 deletions src/internal/components/chart-popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ export interface ChartPopoverProps extends PopoverProps {
trackKey?: string | number;
minVisibleBlockSize?: number;

/** Optional parent element to clamp the popover trigger position within its bounds */
parentRef?: React.RefObject<HTMLElement>;

/** Optional container element that prevents any clicks in there from dismissing the popover */
container: Element | null;

Expand Down Expand Up @@ -74,6 +77,7 @@ function ChartPopover(
trackKey,
onDismiss,
container,
parentRef,
minVisibleBlockSize,

onMouseEnter,
Expand Down Expand Up @@ -137,6 +141,7 @@ function ChartPopover(
trackRef={trackRef}
getTrack={getTrack}
trackKey={trackKey}
parentRef={parentRef}
minVisibleBlockSize={minVisibleBlockSize}
arrow={position => (
<div className={clsx(popoverStyles.arrow, popoverStyles[`arrow-position-${position}`])}>
Expand Down
14 changes: 14 additions & 0 deletions src/popover/__tests__/popover-container.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,17 @@ test.each([null, document.createElement('div')])('accepts track element with get
const getTrack = usePopoverPositionSpy.mock.calls[0][0].getTrack;
expect(getTrack()).toBe(track);
});

test.each([null, document.createElement('div')])('passes parentRef=%s to usePopoverPosition', parentRef => {
render(
<PopoverContainer
{...defaultProps}
parentRef={{ current: parentRef }}
trackRef={{ current: document.createElement('div') }}
>
content
</PopoverContainer>
);

expect(usePopoverPositionSpy.mock.calls[0][0].parentRef?.current).toBe(parentRef);
});
46 changes: 46 additions & 0 deletions src/popover/__tests__/positions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0
import {
calculatePosition,
clampRectStart,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs to be updated to the new function name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for missing this

intersectRectangles,
isCenterOutside,
PRIORITY_MAPPING,
Expand Down Expand Up @@ -280,3 +281,48 @@ describe('isCenterOutside', () => {
).toBe(false);
});
});

describe('clampRectStart', () => {
const parent = { insetInlineStart: 100, insetBlockStart: 100, inlineSize: 400, blockSize: 300 };

test('clamps insetInlineStart to parent start when track is before parent', () => {
const track = { insetInlineStart: 50, insetBlockStart: 200, inlineSize: 20, blockSize: 20 };
expect(clampRectStart(track, parent).insetInlineStart).toBe(100);
});

test('clamps insetBlockStart to parent start when track is above parent', () => {
const track = { insetInlineStart: 200, insetBlockStart: 50, inlineSize: 20, blockSize: 20 };
expect(clampRectStart(track, parent).insetBlockStart).toBe(100);
});

test('clamps insetInlineStart to parent end when track is past parent', () => {
const track = { insetInlineStart: 600, insetBlockStart: 200, inlineSize: 20, blockSize: 20 };
expect(clampRectStart(track, parent).insetInlineStart).toBe(500);
});

test('clamps insetBlockStart to parent end when track is below parent', () => {
const track = { insetInlineStart: 200, insetBlockStart: 500, inlineSize: 20, blockSize: 20 };
expect(clampRectStart(track, parent).insetBlockStart).toBe(400);
});

test('does not clamp when track is within parent bounds', () => {
const track = { insetInlineStart: 200, insetBlockStart: 250, inlineSize: 20, blockSize: 20 };
const result = clampRectStart(track, parent);
expect(result.insetInlineStart).toBe(200);
expect(result.insetBlockStart).toBe(250);
});

test('clamps both axes simultaneously', () => {
const track = { insetInlineStart: 10, insetBlockStart: 500, inlineSize: 20, blockSize: 20 };
const result = clampRectStart(track, parent);
expect(result.insetInlineStart).toBe(100);
expect(result.insetBlockStart).toBe(400);
});

test('preserves inlineSize and blockSize of the track', () => {
const track = { insetInlineStart: 50, insetBlockStart: 50, inlineSize: 20, blockSize: 30 };
const result = clampRectStart(track, parent);
expect(result.inlineSize).toBe(20);
expect(result.blockSize).toBe(30);
});
});
4 changes: 4 additions & 0 deletions src/popover/container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ interface PopoverContainerProps {
*/
trackKey?: string | number;
minVisibleBlockSize?: number;
/** Optional parent element to clamp popover position within its bounds */
parentRef?: React.RefObject<HTMLElement>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not blocking, but this prop name does not convey very well what it is used for. What about something like triggerClipRef, triggerBoundaryRef or triggerClampRef?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's go with triggerClampRef :) Thanks for the suggestion!

position: PopoverProps.Position;
zIndex?: React.CSSProperties['zIndex'];
arrow: (position: InternalPosition | null) => React.ReactNode;
Expand All @@ -52,6 +54,7 @@ export default function PopoverContainer({
trackRef,
getTrack: externalGetTrack,
trackKey,
parentRef,
minVisibleBlockSize,
arrow,
children,
Expand Down Expand Up @@ -90,6 +93,7 @@ export default function PopoverContainer({
popoverRef,
bodyRef,
arrowRef,
parentRef,
getTrack: getTrack.current,
contentRef,
allowScrollToFit,
Expand Down
25 changes: 21 additions & 4 deletions src/popover/use-popover-position.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,20 @@ import {
scrollRectangleIntoView,
} from '../internal/utils/scrollable-containers';
import { BoundingBox, InternalPosition, Offset, PopoverProps, Rect } from './interfaces';
import { calculatePosition, getDimensions, getOffsetDimensions, isCenterOutside } from './utils/positions';
import {
calculatePosition,
clampRectStart,
getDimensions,
getOffsetDimensions,
isCenterOutside,
} from './utils/positions';

export default function usePopoverPosition({
popoverRef,
bodyRef,
arrowRef,
getTrack,
parentRef,
contentRef,
allowScrollToFit,
allowVerticalOverflow,
Expand All @@ -34,6 +41,7 @@ export default function usePopoverPosition({
arrowRef: React.RefObject<HTMLDivElement | null>;
getTrack: () => null | HTMLElement | SVGElement;
contentRef: React.RefObject<HTMLDivElement | null>;
parentRef?: React.RefObject<HTMLElement>;
allowScrollToFit?: boolean;
allowVerticalOverflow?: boolean;
preferredPosition: PopoverProps.Position;
Expand Down Expand Up @@ -88,7 +96,7 @@ export default function usePopoverPosition({
// Get rects representing key elements
// Use getComputedStyle for arrowRect to avoid modifications made by transform
const viewportRect = getViewportRect(document.defaultView!);
const trackRect = getLogicalBoundingClientRect(track);
const trackRect = getClampedTrackRect(track, parentRef?.current);
const arrowRect = getDimensions(arrow);
const { containingBlock, boundary } = findUpUntilMultiple({
startElement: popover,
Expand Down Expand Up @@ -183,7 +191,7 @@ export default function usePopoverPosition({
if (!track) {
return;
}
const trackRect = getLogicalBoundingClientRect(track);
const trackRect = getClampedTrackRect(track, parentRef?.current);

const newTrackOffset = toRelativePosition(
trackRect,
Expand All @@ -209,13 +217,14 @@ export default function usePopoverPosition({
bodyRef,
contentRef,
arrowRef,
parentRef,
keepPosition,
preferredPosition,
renderWithPortal,
allowVerticalOverflow,
minVisibleBlockSize,
allowScrollToFit,
hideOnOverscroll,
minVisibleBlockSize,
]
);
return { updatePositionHandler, popoverStyle, internalPosition, positionHandlerRef, isOverscrolling };
Expand Down Expand Up @@ -262,3 +271,11 @@ function isBoundary(element: HTMLElement) {
const computedStyle = getComputedStyle(element);
return !!computedStyle.clipPath && computedStyle.clipPath !== 'none';
}

function getClampedTrackRect(track: HTMLElement | SVGElement, parentRef?: HTMLElement | null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add unit testing coverage for this new logic?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure! I just wanted to get your feedback on the implementation first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Note, Codecov is still complaining about line 280 below not being covered. You might want to either add coverage for getClampedTrackRect specifically, or some case(s) for the popover component using a value for parent (if not too complicated, I'd personally prefer the latter, because it would test the integration of the logic in the component rather than in isolation).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with the latter approach as well!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adding the test, although Codecov still complains about line 280 not being covered. Could this be because by asserting

  expect(usePopoverPositionSpy.mock.calls[0][0].parentRef?.current).toBe(parentRef);

you are only ever checking the first call, which corresponds to when the ref is null?

const trackRect = getLogicalBoundingClientRect(track);
if (!parentRef) {
return trackRect;
}
return clampRectStart(trackRect, getLogicalBoundingClientRect(parentRef));
}
8 changes: 8 additions & 0 deletions src/popover/utils/positions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,14 @@ function isTopOrBottom(internalPosition: InternalPosition) {
return ['top', 'bottom'].includes(internalPosition.split('-')[0]);
}

export function clampRectStart(rect: Rect, bounds: BoundingBox) {
const parentInlineEnd = bounds.insetInlineStart + bounds.inlineSize;
const parentBlockEnd = bounds.insetBlockStart + bounds.blockSize;
rect.insetInlineStart = Math.max(bounds.insetInlineStart, Math.min(rect.insetInlineStart, parentInlineEnd));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any particular reason to modify the original passed-in object rather than creating a new one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking again at the function I preferred to have it mutate the argument instead of creating a new one. I know this is mostly a matter of personal preference so if you prefer to create the new object I'm also fine with that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general it is preferred to not mutate objects, because from within the function you don't know what else is the object is used for, so its mutation might be unexpected to a user outside of it, leading to hard-to-debug issues.

rect.insetBlockStart = Math.max(bounds.insetBlockStart, Math.min(rect.insetBlockStart, parentBlockEnd));
return rect;
}

export function isCenterOutside(child: Rect, parent: Rect) {
const childCenter = child.insetBlockStart + child.blockSize / 2;
const overflowsBlockStart = childCenter < parent.insetBlockStart;
Expand Down
Loading