Horizontal scrolling by daun · Pull Request #93 · swup/scroll-plugin

daun · 2025-06-08T20:52:44Z

Description

Scroll containers horizontally to reveal anchors
Change API of scrollTo from just number to number | { top: number, left: number }
Normalize a number returned from OffsetCallback to { top: number, left: 0}
Communicate that the OffsetCallback arguments are never undefined
Pass the target position to OffsetCallback to provide fine-grained control

Example from playground

Checks

The PR is submitted to the next branch
The code was linted before pushing (npm run lint)
~~All tests are passing (npm run test)~~
~~New or updated tests are included~~
The documentation was updated as required

Signed-off-by: Philipp Daun <post@philippdaun.net>

github-actions · 2025-06-08T20:54:23Z

Playwright test results

3 passed

Details

3 tests across 1 suite

363b3cd

Signed-off-by: Philipp Daun <post@philippdaun.net>

Copilot

Pull Request Overview

Adds horizontal scrolling support and updates the scrollTo API to accept an object with both top and left offsets.

Change swup.scrollTo signature to number | { top, left }
Update internal calls and implementation to handle both axes
Bind plugin’s scrollTo method directly to the Swup instance

Comments suppressed due to low confidence (3)

src/index.ts:405

[nitpick] Update the doc comment to mention both horizontal and vertical offsets, for example: "Scroll to a specific top & left offset, with optional animation."

* Scroll to a specific offset, with optional animation.

src/index.ts:33

New horizontal scrolling logic and API signature change should be covered by unit tests. Consider adding tests for scrollTo({ top, left }) and mixed number/object inputs.

scrollTo?: (
			position: number | ScrollPosition,

src/index.ts:465

getOffset likely only returns a vertical offset. Subtracting it from left may produce incorrect horizontal positioning. Consider calculating a separate horizontal offset or confirm that getOffset handles both axes.

this.scrollTo({ top: top - offset, left: left - offset }, animate, scrollContainer);

src/index.ts

hirasso · 2025-06-09T16:32:43Z

Nice! Did you activate those Copilot reviews? I'm only on mobile right now, but the point about offset currently only being for top offset looks like it makes sense.

daun · 2025-06-09T16:43:20Z

I asked him for fun :) Haven't looked at it, but makes sense to allow setting both top and left offsets if useful.

hirasso · 2025-06-09T16:57:41Z

For e2e tests - what do you think about using your playground for the fixtures? It's also astro, after all.

daun · 2025-06-09T17:00:50Z

For e2e tests - what do you think about using your playground for the fixtures? It's also astro, after all.

Sure, we can copy them over. However the styling is a bit messy, so we might even be faster by just recreating the styles.

daun · 2025-06-09T17:01:11Z

Added support for vertical + horizontal offsets.

hirasso · 2025-06-09T17:15:15Z

src/index.ts

+			return offset;
+		} else {
+			const top = parseInt(String(offset ?? ''), 10) || 0;
+			return { top, left: top };


If we treat a single number as top in scrollTo, I'd expect a single number for offset also to be treated as the top offset:

return { top, left: 0 };

Interesting. I'd expect it to have a uniform offset. Mentally, I see it a bit like the scroll-margin css property, which applies to all sides when set to a single value. Whereas scrolling always requires being explicit like in window.scrollTo(x, y)...

Hmm. This would break expectations with how the offset used to work, at least for me.

But let's take a step back - would it make sense to start respecting the actual scroll-margin css value instead of the js version we provide right now? And only provide the offset callback? I don't know, happy to give this some more thought in a talk.

From MDN:

The scroll-margin-top property defines the top margin of the scroll snap area that is used for snapping this box to the snapport.

~~So scroll-margin only seems to be related to scroll snapping, not to anchor scrolling, if I read this right? I have never used scroll-margin-* myself.~~ Scratch that, it applies to anchor scrolling without any snapping as well – tested it myself just now.

I'd love to use scroll-margin exclusively, however it's not perfect. The package we're using to compute scroll movements does calculate it in, so it's good to go in that sense. The issue is with the actual spec/browser implementation. There's no way of defining cumulative offsets when nesting scroll containers. So I can't define an offset for scrolling the body (e.g. the header height) and a different offset for scrolling inside overflowing containers (e.g. 1em). Happy to show you in a call how it behaves in the playground.

All that to say: it's a great idea to use scroll-margin for 90% of use cases. Just when targeting elements inside scroll containers, it gets tricky and requires some custom JS logic.

Ah yes, scroll margin applies to all scrolling. Took me a while to figure that out myself :)

I'd love to use scroll-margin exclusively, however it's not perfect. The package we're using to compute scroll movements does calculate it in, so it's good to go in that sense. The issue is with the actual spec/browser implementation. There's no way of defining cumulative offsets when nesting scroll containers. So I can't define an offset for scrolling the body (e.g. the header height) and a different offset for scrolling inside overflowing containers (e.g. 1em). Happy to show you in a call how it behaves in the playground.

Yes please, I would love that!

hirasso · 2025-06-09T17:18:31Z

For e2e tests - what do you think about using your playground for the fixtures? It's also astro, after all.

Sure, we can copy them over. However the styling is a bit messy, so we might even be faster by just recreating the styles.

Ok, maybe recreating the styles is the better idea. I used tailwind for the fixtures, which helps a lot with creating multiple independent pages / one-off styles.

hirasso · 2025-06-09T20:23:31Z

src/index.ts

+	offset:
+		| number
+		| ScrollPosition
+		| ((scrollTarget?: Element, scrollContainer?: Element) => number | ScrollPosition);


I just stumbled upon this: Are scrollTarget and scrollContainer not always defined? getOffset is only being called by scrollElementIntoView(), where both are always defined.

I'm currently confused – is the offset to be applied when doing a normal scroll to top (without a scrollTarget)? If so, getOffset should be changed to this:

((scrollContainer: Element, scrollTarget?: Element) => number | ScrollPosition)

True, these should always be defined. We can probably safely change the signature.

The offset is only applied when scrolling to an element. When scrolling to the top, it wouldn't have an effect. Subtracting the offset would end up with a scroll position below zero, so still technically the top.

I have no idea why we made these arguments optional. Maybe I bungled it while adding the feature :)

Thank you for clearing that up with the negative scroll offset 😅🫠

daun · 2025-06-10T09:04:32Z

Note from recent call: make offset option function args required, and fall back to left: 0 for scalar offset values.

hirasso · 2025-06-10T09:13:12Z

...and: clamp offsets so that the scroll target will be revealed completely, even if at the far edges of the scroll container.

netlify · 2025-06-11T09:35:35Z

✅ Deploy Preview for swup-scroll-plugin ready!

Name	Link
🔨 Latest commit	`363b3cd`
🔍 Latest deploy log	https://app.netlify.com/projects/swup-scroll-plugin/deploys/684956a8bbf7c40008ed9806
😎 Deploy Preview	https://deploy-preview-93--swup-scroll-plugin.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

…ined

This makes it possible for consumers what to do when scrolling to the far edges of a container (should the an offset be substracted or not)

hirasso · 2025-06-11T10:11:48Z

I've implemented the last missing pieces and added them to the top comment:

Normalize a number returned from OffsetCallback to { top: number, left: 0}
Communicate that the OffsetCallback arguments are never undefined
Pass the target position to OffsetCallback to provide fine-grained control
Export the type OffsetCallback to make it easier to decouple the code from the plugin instantiation

The third is the best I could come up with to enable users to clamp offsets so that the scroll target will be revealed completely, even if at the far edges of the scroll container. I think it's best to not make any assumptions about what everybody wants here. In the playground code, I could then return 0 if the target position was at the max available scroll height:

/**
 * Do not offset the scroll if scrolling to the very end
 */
const maxTop = container.scrollHeight - container.clientHeight;
if (top === maxTop) return 0;

What do you think?

Implement horizontal scroll

effcd5b

Signed-off-by: Philipp Daun <post@philippdaun.net>

Scroll to top left

b708d0c

Signed-off-by: Philipp Daun <post@philippdaun.net>

daun requested review from a team and Copilot June 8, 2025 21:21

Copilot AI reviewed Jun 8, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

daun linked an issue Jun 8, 2025 that may be closed by this pull request

Support horizontal scrolling #94

Closed

4 tasks

daun mentioned this pull request Jun 8, 2025

Next #90

Merged

13 tasks

Support vertical and horizontal offsets

96caa6c

Ensure scroll position defaults

cd52ba2

hirasso reviewed Jun 9, 2025

View reviewed changes

hirasso added 3 commits June 9, 2025 20:55

Fixtures WIP

93d691e

Add nested scrolling links

fbcca30

WIP

7d87bbb

hirasso reviewed Jun 9, 2025

View reviewed changes

hirasso added 4 commits June 9, 2025 22:36

WIP

4947dc4

Cleanup

1accd13

Move ./tests/e2e/fixtures to ./playground

9ba4951

WIP

d560981

hirasso added 3 commits June 10, 2025 11:45

WIP

b272d1b

Add data-swup-scroll-container attributes for e2e tests

f1f9467

Prepare for scroll preservation tests

bd20bb6

hirasso added 2 commits June 10, 2025 12:33

Use an attribute [data-preserve-scroll] instead of a class name

75bc131

Clean up

fa2a05e

hirasso mentioned this pull request Jun 10, 2025

Better fixtures #95

Closed

5 tasks

hirasso added 2 commits June 11, 2025 11:30

Remove unnessesary check for empty scrollTarget

3800449

Set left to zero if the provided offset value is a number

bcf45c8

hirasso added 3 commits June 11, 2025 11:36

Communicate that scrollTarget and scrollContainer are never undef…

25ec9da

…ined

Simplify playground offset callback

729a72f

Pass the position to OffsetCallback

de3ab2e

This makes it possible for consumers what to do when scrolling to the far edges of a container (should the an offset be substracted or not)

Export the type OffsetCallback

363b3cd

hirasso merged commit b631bce into next Jun 12, 2025
6 checks passed

hirasso deleted the feat/horizontal-scrolling branch June 12, 2025 13:35

Uh oh!

Conversation

daun commented Jun 8, 2025 • edited by hirasso Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Playwright test results

Details

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

hirasso commented Jun 9, 2025

Uh oh!

daun commented Jun 9, 2025

Uh oh!

hirasso commented Jun 9, 2025

Uh oh!

daun commented Jun 9, 2025

Uh oh!

daun commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hirasso Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hirasso commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daun commented Jun 10, 2025

Uh oh!

hirasso commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for swup-scroll-plugin ready!

Uh oh!

hirasso commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daun commented Jun 8, 2025 •

edited by hirasso

Loading

github-actions bot commented Jun 8, 2025 •

edited

Loading

hirasso Jun 9, 2025 •

edited

Loading

hirasso commented Jun 9, 2025 •

edited

Loading

hirasso commented Jun 10, 2025 •

edited

Loading

netlify bot commented Jun 11, 2025 •

edited

Loading

hirasso commented Jun 11, 2025 •

edited

Loading