Skip to content

Commit 550a54c

Browse files
authored
fix(issues): Switch from scrollIntoView to scrollTo (#76248)
1 parent 41afb8f commit 550a54c

File tree

2 files changed

+43
-20
lines changed

2 files changed

+43
-20
lines changed

static/app/components/scrollCarousel.spec.tsx

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,14 @@ describe('ScrollCarousel', function () {
4141

4242
it('shows right arrow when elements exist to the right', async function () {
4343
render(
44-
<ScrollCarousel>
44+
<ScrollCarousel data-test-id="scroll">
4545
<div data-test-id="child-1" />
4646
<div data-test-id="child-2" />
4747
<div data-test-id="child-3" />
4848
</ScrollCarousel>
4949
);
5050

51+
const scrollContainer = screen.getByTestId('scroll');
5152
const elements = [
5253
screen.getByTestId('child-1'),
5354
screen.getByTestId('child-2'),
@@ -66,21 +67,22 @@ describe('ScrollCarousel', function () {
6667
const rightButton = screen.getByRole('button', {name: 'Scroll right'});
6768
expect(screen.queryByRole('button', {name: 'Scroll left'})).not.toBeInTheDocument();
6869

69-
// Test scroll into view, the last element should have its 'scrollIntoView' called
70-
elements.at(-1)!.scrollIntoView = jest.fn();
70+
// Test scroll into view, the last element should have its 'scrollTo' called
71+
scrollContainer.scrollTo = jest.fn();
7172
await userEvent.click(rightButton);
72-
expect(elements.at(-1)!.scrollIntoView).toHaveBeenCalled();
73+
expect(scrollContainer.scrollTo).toHaveBeenCalled();
7374
});
7475

7576
it('shows left arrow when elements exist to the left', async function () {
7677
render(
77-
<ScrollCarousel>
78+
<ScrollCarousel data-test-id="scroll">
7879
<div data-test-id="child-1" />
7980
<div data-test-id="child-2" />
8081
<div data-test-id="child-3" />
8182
</ScrollCarousel>
8283
);
8384

85+
const scrollContainer = screen.getByTestId('scroll');
8486
const elements = [
8587
screen.getByTestId('child-1'),
8688
screen.getByTestId('child-2'),
@@ -99,9 +101,9 @@ describe('ScrollCarousel', function () {
99101
const leftButton = await screen.findByRole('button', {name: 'Scroll left'});
100102
expect(screen.queryByRole('button', {name: 'Scroll right'})).not.toBeInTheDocument();
101103

102-
// Test scroll into view, the 1st element should have its 'scrollIntoView' called
103-
elements[0].scrollIntoView = jest.fn();
104+
// Test scroll into view, the 1st element should have its 'scrollTo' called
105+
scrollContainer.scrollTo = jest.fn();
104106
await userEvent.click(leftButton);
105-
expect(elements[0].scrollIntoView).toHaveBeenCalled();
107+
expect(scrollContainer.scrollTo).toHaveBeenCalled();
106108
});
107109
});

static/app/components/scrollCarousel.tsx

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {useRefChildrenVisibility} from 'sentry/utils/useRefChildrenVisibility';
1212
interface ScrollCarouselProps {
1313
children: React.ReactNode;
1414
className?: string;
15+
'data-test-id'?: string;
1516
gap?: ValidSize;
1617
}
1718

@@ -32,7 +33,26 @@ const DEFAULT_VISIBLE_RATIO = 0.85;
3233
*/
3334
const DEFAULT_JUMP_ITEM_COUNT = 2;
3435

35-
export function ScrollCarousel({children, className, gap = 1}: ScrollCarouselProps) {
36+
/**
37+
* Calculates the offset rectangle of an element relative to another element.
38+
*/
39+
const getOffsetRect = (el: HTMLElement, relativeTo: HTMLElement) => {
40+
const rect = el.getBoundingClientRect();
41+
if (!relativeTo) {
42+
return rect;
43+
}
44+
const relativeRect = relativeTo.getBoundingClientRect();
45+
return {
46+
left: rect.left - relativeRect.left,
47+
top: rect.top - relativeRect.top,
48+
right: rect.right - relativeRect.left,
49+
bottom: rect.bottom - relativeRect.top,
50+
width: rect.width,
51+
height: rect.height,
52+
};
53+
};
54+
55+
export function ScrollCarousel({children, gap = 1, ...props}: ScrollCarouselProps) {
3656
const scrollContainerRef = useRef<HTMLDivElement | null>(null);
3757
const {visibility, childrenEls} = useRefChildrenVisibility({
3858
children,
@@ -47,10 +67,11 @@ export function ScrollCarousel({children, className, gap = 1}: ScrollCarouselPro
4767
const scrollIndex = visibility.findIndex(Boolean);
4868
// Clamp the scroll index to the first visible item
4969
const clampedIndex = Math.max(scrollIndex - DEFAULT_JUMP_ITEM_COUNT, 0);
50-
childrenEls[clampedIndex]?.scrollIntoView({
70+
// scrollIntoView scrolls the entire page on some browsers
71+
scrollContainerRef.current?.scrollTo({
5172
behavior: 'smooth',
52-
block: 'nearest',
53-
inline: 'start',
73+
// We don't need to do any fancy math for the left edge
74+
left: getOffsetRect(childrenEls[clampedIndex], childrenEls[0]).left,
5475
});
5576
}, [visibility, childrenEls]);
5677

@@ -61,20 +82,20 @@ export function ScrollCarousel({children, className, gap = 1}: ScrollCarouselPro
6182
scrollIndex + DEFAULT_JUMP_ITEM_COUNT,
6283
visibility.length - 1
6384
);
64-
childrenEls[clampedIndex]?.scrollIntoView({
85+
86+
const targetElement = childrenEls[clampedIndex];
87+
const targetElementRight = getOffsetRect(targetElement, childrenEls[0]).right;
88+
const containerRight = scrollContainerRef.current?.clientWidth ?? 0;
89+
// scrollIntoView scrolls the entire page on some browsers
90+
scrollContainerRef.current?.scrollTo({
6591
behavior: 'smooth',
66-
block: 'nearest',
67-
inline: 'end',
92+
left: Math.max(targetElementRight - containerRight, 0),
6893
});
6994
}, [visibility, childrenEls]);
7095

7196
return (
7297
<ScrollCarouselWrapper>
73-
<ScrollContainer
74-
ref={scrollContainerRef}
75-
className={className}
76-
style={{gap: space(gap)}}
77-
>
98+
<ScrollContainer ref={scrollContainerRef} style={{gap: space(gap)}} {...props}>
7899
{children}
79100
</ScrollContainer>
80101
{!isAtStart && <LeftMask />}

0 commit comments

Comments
 (0)