Skip to content

Commit 3cb5377

Browse files
authored
[Scrollable] fix: scrollable calls onScrolledToBottom only when container is overflowing (#7635)
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? Fixes #0000 <!-- link to issue if one exists --> <!-- Context about the problem that’s being addressed. --> ### WHAT is this pull request doing? <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) In storybook, create a scrollable component with a small amount of text so that it doesn't scroll. Make sure the `onScrolledToBottom` callback is **not** fired when the scrollable component renders. On `main` the opposite should be true. ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
1 parent b52e367 commit 3cb5377

File tree

3 files changed

+79
-1
lines changed

3 files changed

+79
-1
lines changed

.changeset/thick-clocks-applaud.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/polaris': patch
3+
---
4+
5+
Fixed Scrollable component to match existing onScrolledToBottom logic

polaris-react/src/components/Scrollable/Scrollable.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ export function Scrollable({
6767

6868
requestAnimationFrame(() => {
6969
const {scrollTop, clientHeight, scrollHeight} = currentScrollArea;
70+
const canScroll = Boolean(scrollHeight > clientHeight);
7071
const isBelowTopOfScroll = Boolean(scrollTop > 0);
7172
const isAtBottomOfScroll = Boolean(
7273
scrollTop + clientHeight >= scrollHeight - LOW_RES_BUFFER,
@@ -75,7 +76,7 @@ export function Scrollable({
7576
setTopShadow(isBelowTopOfScroll);
7677
setBottomShadow(!isAtBottomOfScroll);
7778

78-
if (isAtBottomOfScroll && onScrolledToBottom) {
79+
if (canScroll && isAtBottomOfScroll && onScrolledToBottom) {
7980
onScrolledToBottom();
8081
}
8182
});

polaris-react/src/components/Scrollable/tests/Scrollable.test.tsx

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,24 @@ import {mountWithApp} from 'tests/utilities';
33

44
import {Scrollable} from '../Scrollable';
55
import {ScrollableContext} from '../context';
6+
import type {ScrollableProps} from '../Scrollable';
67

78
describe('<Scrollable />', () => {
9+
let rafSpy: jest.SpyInstance;
10+
11+
beforeAll(() => {
12+
rafSpy = jest
13+
.spyOn(window, 'requestAnimationFrame')
14+
.mockImplementation((cb) => {
15+
cb(Date.now());
16+
return Math.random();
17+
});
18+
});
19+
20+
afterAll(() => {
21+
rafSpy.mockRestore();
22+
});
23+
824
it('mounts', () => {
925
const scrollable = mountWithApp(<Scrollable />);
1026
expect(scrollable).toBeDefined();
@@ -71,4 +87,60 @@ describe('<Scrollable />', () => {
7187
tabIndex: 0,
7288
});
7389
});
90+
91+
it('calls onScrolledToBottom when scrolled to bottom', () => {
92+
const onScrolledToBottom = jest.fn();
93+
94+
const scrollArea = mountWithApp(
95+
<Scrollable
96+
data-test-id="scroll-element"
97+
onScrolledToBottom={onScrolledToBottom}
98+
>
99+
<p>Hello</p>
100+
</Scrollable>,
101+
);
102+
103+
const scrollNode = scrollArea.find('div', {
104+
'data-test-id': 'scroll-element',
105+
} as ScrollableProps)?.domNode!;
106+
107+
// defineProperty needed to assign values to readonly node properties
108+
Object.defineProperty(scrollNode, 'clientHeight', {get: () => 0});
109+
Object.defineProperty(scrollNode, 'scrollHeight', {get: () => 10});
110+
Object.defineProperty(scrollNode, 'scrollTop', {get: () => 10});
111+
112+
scrollArea.act(() => {
113+
scrollNode.dispatchEvent(new Event('scroll'));
114+
});
115+
116+
expect(onScrolledToBottom).toHaveBeenCalledTimes(1);
117+
});
118+
119+
it(`doesn't call onScrolledToBottom when the scroll area is not overflowing`, () => {
120+
const onScrolledToBottom = jest.fn();
121+
122+
const scrollArea = mountWithApp(
123+
<Scrollable
124+
data-test-id="scroll-element"
125+
onScrolledToBottom={onScrolledToBottom}
126+
>
127+
<p>Hello</p>
128+
</Scrollable>,
129+
);
130+
131+
const scrollNode = scrollArea.find('div', {
132+
'data-test-id': 'scroll-element',
133+
} as ScrollableProps)?.domNode!;
134+
135+
// defineProperty needed to assign values to readonly node properties
136+
Object.defineProperty(scrollNode, 'clientHeight', {get: () => 10});
137+
Object.defineProperty(scrollNode, 'scrollHeight', {get: () => 10});
138+
Object.defineProperty(scrollNode, 'scrollTop', {get: () => 10});
139+
140+
scrollArea.act(() => {
141+
scrollNode.dispatchEvent(new Event('scroll'));
142+
});
143+
144+
expect(onScrolledToBottom).not.toHaveBeenCalled();
145+
});
74146
});

0 commit comments

Comments
 (0)