Skip to content

feat: allow skipping scrolling on partial navigation #3817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CertainLach wants to merge 2 commits into
base: main
Choose a base branch
from
+4 −1
Open

feat: allow skipping scrolling on partial navigation #3817

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1adec89
feat: allow skipping scrolling on partial navigation
CertainLach May 21, 2026
7fab8b2
Merge branch 'main' into push-vxnsyvkpytzk
bartlomieju May 26, 2026
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: 4 additions & 1 deletion packages/fresh/src/runtime/client/partials.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import { INTERNAL_PREFIX, PARTIAL_SEARCH_PARAM } from "../../constants.ts";

export const PARTIAL_ATTR = "f-partial";
export const PARTIAL_NO_SCROLL_ATTR = "f-dont-scroll";
Copy link
Copy Markdown
Contributor

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

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

Could we rename this to f-no-scroll? It's a bit out of step with the rest of Fresh's attribute vocabulary (f-partial, f-client-nav) — all existing ones are positive nouns without contractions.

Suggested change
export const PARTIAL_NO_SCROLL_ATTR = "f-dont-scroll";
export const PARTIAL_NO_SCROLL_ATTR = "f-no-scroll";


class NoPartialsError extends Error {}

Expand Down Expand Up @@ -170,7 +171,9 @@
await fetchPartials(nextUrl, partialUrl, true);
updateLinks(nextUrl);
});
scrollTo({ left: 0, top: 0, behavior: "instant" });
if (!el.hasAttribute(PARTIAL_NO_SCROLL_ATTR)) {
Copy link
Copy Markdown
Contributor

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

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

Minor: this reads the attribute off the anchor itself, not ancestors — that's consistent with how f-partial is read here, so I think it's fine, but worth calling out in the docs so users know to put the attribute on the <a> rather than a wrapper element.

Copy link
Copy Markdown
Contributor Author

@CertainLach CertainLach May 26, 2026

Choose a reason for hiding this comment

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

Actually, thinking about it, I wonder if f-partial on should not scroll to top by default at all

Because I don't think I have found any use-case when , a mechanism for explicitly updating a PART of the page needs to scroll

Copy link
Copy Markdown
Contributor

@bartlomieju bartlomieju May 27, 2026

Choose a reason for hiding this comment

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

I'd lean toward shipping this as-is (opt-out) rather than flipping the default in this PR.

Changing scroll-to-top to off-by-default is a behavior change every existing <a f-partial> user inherits silently. There are legitimate page-level uses today — pagination links, filter changes on long lists, "load next" style flows — where the current scroll-to-top is what users expect. Removing it without warning would leave those users stranded mid-page after a click, which is just as disorienting as the inverse problem you're solving here.

The opt-out shape (f-partial-no-scroll) is also additive and safe: anyone who wants the new behavior adds the attribute, nobody else is affected. Flipping the default is the kind of change that wants its own PR with a changelog entry and probably an inverse f-partial-scroll attribute for the page-level cases — worth a separate discussion with maintainers rather than bundling here.

Happy to revisit the default in a follow-up if there's appetite for it.

scrollTo({ left: 0, top: 0, behavior: "instant" });
}
} finally {
if (indicator !== undefined) {
indicator.value = false;
Expand Down Expand Up @@ -415,7 +418,7 @@
for (let i = 0; i < json.islands.length; i++) {
const island = json.islands[i];
promises.push(
import(/* @vite-ignore */ island.chunk).then((mod) => {

Check warning on line 421 in packages/fresh/src/runtime/client/partials.ts

View workflow job for this annotation

GitHub Actions / test (v2.x, ubuntu-latest) 

unable to analyze dynamic import
ISLAND_REGISTRY.set(island.name, mod[island.exportName]);
}),
);
Expand Down
Loading