feat: allow skipping scrolling on partial navigation#3817
Conversation
CertainLach
force-pushed
the
push-vxnsyvkpytzk
branch
from
c287613 to
1adec89
Compare
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks for the PR — the use case is convincing and the diff-perspective example makes the motivation clear. A few requests before we merge (left some inline). Headline one: I'd suggest renaming the attribute to f-no-scroll to match Fresh's existing attribute vocabulary (f-partial, f-client-nav) — they're all positive nouns without contractions.
It would also be great to:
- Add a short section to
docs/latest/advanced/partials.mddocumenting the new attribute, otherwise nobody will discover it. - Add a regression test in
packages/fresh/tests/partials_test.tsxasserting that scroll position is preserved when the attribute is present (and reset when it isn't).
Form-submit and button partials don't currently scroll, so anchors-only feels like the right scope for now.
| import { INTERNAL_PREFIX, PARTIAL_SEARCH_PARAM } from "../../constants.ts"; | ||
|
|
||
| export const PARTIAL_ATTR = "f-partial"; | ||
| export const PARTIAL_NO_SCROLL_ATTR = "f-dont-scroll"; |
There was a problem hiding this comment.
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.
| export const PARTIAL_NO_SCROLL_ATTR = "f-dont-scroll"; | |
| export const PARTIAL_NO_SCROLL_ATTR = "f-no-scroll"; |
| updateLinks(nextUrl); | ||
| }); | ||
| scrollTo({ left: 0, top: 0, behavior: "instant" }); | ||
| if (!el.hasAttribute(PARTIAL_NO_SCROLL_ATTR)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
2 participants
When using partials to update a small portion of the page, scroll might be undesired
For example, at https://git.delta.rocks/r/jrsonnet/-/rev/7bd5bbd0756a11586a0684061e14507b6c0f7d14/change (A git repo viewer project built with fresh, ignore the code that is being displayed inside)
Go to the bottom, to the diff for file
crates/jrsonnet-evaluator/src/tla.rsHere in the top right corner for this file you can change "perspective" between diff/before/after/both, partials are used to only update perspective for this file, and having it scroll back to top is very undesired. The site already has this patch applied to make it behave as intended, the attribute can be dropped from html to see how it should not behave.