Skip to content

Commit 33f379f

Browse files
authored
CORE-2604 Fix scroll loop (#133)
* Prevent scroll loop by using useLayoutEffect * Prevent useMatchMediaQuery from removing and re-attaching its change listeners on every render * Actually handle changing matchMedia.matches * Version bump
1 parent 1866a57 commit 33f379f

5 files changed

Lines changed: 17 additions & 6 deletions

File tree

package-lock.json

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@openstax/ui-components",
3-
"version": "1.23.4",
3+
"version": "1.23.5",
44
"license": "MIT",
55
"repository": "https://github.com/openstax/ui-components.git",
66
"publishConfig": {

src/components/SidebarNav/hooks.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,10 @@ export const useScrollRestoration = (
116116
) => {
117117
const [scrollPosition, setScrollPosition] = React.useState(0);
118118

119-
// Restore scroll position after render
120-
requestAnimationFrame(() => {
119+
// useLayoutEffect without deps runs after every render, before paint,
120+
// preventing the re-render loop that a bare requestAnimationFrame in the
121+
// render body causes (each RAF fires a scroll event → state update → loop).
122+
React.useLayoutEffect(() => {
121123
if (ref.current) {
122124
ref.current.scrollTop = scrollPosition;
123125
}

src/hooks.spec.tsx

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,14 @@ describe("useMatchMediaQuery", () => {
9898
expect(screen.getByTestId("result").textContent).toBe("Matches");
9999
});
100100

101+
test("it syncs matches when the query prop changes", () => {
102+
const component = render(<MediaComponent query="(min-width: 600px)" />);
103+
expect(screen.getByTestId("result").textContent).toBe("Matches");
104+
105+
component.rerender(<MediaComponent query="(min-width: 800px)" />);
106+
expect(screen.getByTestId("result").textContent).toBe("Does not match");
107+
});
108+
101109
test("it attaches and detaches event listeners", () => {
102110
const mock = {
103111
addEventListener: jest.fn(),

src/hooks.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export const useSetAppError = () => {
2121
}
2222

2323
export const useMatchMediaQuery = (query: string) => {
24-
const matchMedia = window.matchMedia(query);
24+
const matchMedia = React.useMemo(() => window.matchMedia(query), [query]);
2525
const [matches, setMatches] = React.useState(matchMedia.matches);
2626

2727
const listener = React.useCallback((e: MediaQueryListEvent) => {
@@ -33,6 +33,7 @@ export const useMatchMediaQuery = (query: string) => {
3333
}, []);
3434

3535
React.useEffect(() => {
36+
setMatches(matchMedia.matches);
3637
if (typeof matchMedia.addEventListener === "function") {
3738
matchMedia.addEventListener("change", listener);
3839
} else {

0 commit comments

Comments
 (0)