Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new composable app/composables/useScrollToTop.ts exposing Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
these look like good changes to the scroll composable 👌
my only question is about displaying the button on desktop. it feels like an affordance needed for mobile, but users with a keyboard will have a 'Home' key and an extra button clutters the UI
Would the button appear on tablets ? |
|
it should probably be gated not by window width but by |
Good point. I'll switch to using
Ah, TIL about the Home/End shortcuts! Thanks for the tip. |
|
I've already implemented |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const isTouchDeviceClient = shallowRef(false) | ||
| const isVisible = shallowRef(false) | ||
| const scrollThreshold = 300 | ||
| const { isSupported: supportsScrollStateQueries } = useCssSupports( | ||
| 'container-type', | ||
| 'scroll-state', | ||
| { ssrValue: false }, | ||
| ) | ||
| const shouldShowButton = computed(() => isActive.value && isTouchDeviceClient.value) |
There was a problem hiding this comment.
Desktop visibility still blocked by touch-only gating.
shouldShowButton requires isTouchDeviceClient, so the button never appears on non-touch desktops. That conflicts with issue #1379’s requirement to show the control on desktop. If the requirement still stands, drop the touch-only gate (or make it configurable). If the requirement has changed, please update the issue/PR description to match.
🛠️ Suggested fix if desktop should be supported
-const isTouchDeviceClient = shallowRef(false)
const isVisible = shallowRef(false)
@@
-const shouldShowButton = computed(() => isActive.value && isTouchDeviceClient.value)
+const shouldShowButton = computed(() => isActive.value)
@@
onMounted(() => {
- isTouchDeviceClient.value = isTouchDevice()
onScroll()
})Also applies to: 34-34
There was a problem hiding this comment.
This is ok, requirements changed to only show the button on touch decices
There was a problem hiding this comment.
@graphieros Thank you for clarifying! I understand the requirements have shifted to show the button only on touch devices. The current implementation correctly achieves that.
Since issue #1379 is titled "Back-to-top button on desktop" and the PR description mentions "Desktop Support: Enabled the button on desktop", you may want to update both to reflect this new direction—perhaps explaining that after discussion, the button was decided to remain touch-only to avoid cluttering desktop UI (where keyboard shortcuts like Home/End are available).
✏️ Learnings added
Learnt from: graphieros
Repo: npmx-dev/npmx.dev PR: 1453
File: app/components/ScrollToTop.client.vue:12-20
Timestamp: 2026-02-13T14:11:12.069Z
Learning: For the ScrollToTop button component in app/components/ScrollToTop.client.vue: the button should only be shown on touch devices (gated by `isTouchDevice()`), not on desktop, to avoid cluttering desktop UI where keyboard shortcuts are available.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
looking great! but one more thought - we might want to update the implementation on the package page as we already have a bottom bar - maybe (on that page) build another button into that bottom bar and hide this one? 
|
|
Yes, I also think they should be merged. However, this raises another issue: when switching to other languages, the floating buttons at the bottom extend beyond the screen, and the display changes accordingly (for example, the floating button area becomes higher). I mentioned this issue in PR #1284 before. I'm unsure whether the text content should be hidden on small screens. I haven't thought of a better solution at the moment. Device: iPhone 14 Pro Max - browser (simulated)
|
OrbisK
left a comment
OrbisK
left a comment
There was a problem hiding this comment.
FYI: There is https://vueuse.org/core/useScroll
| class="scroll-to-top-css fixed bottom-4 inset-ie-4 z-50 w-12 h-12 bg-bg-elevated border border-border rounded-full shadow-lg flex items-center justify-center text-fg-muted hover:text-fg transition-colors active:scale-95" | ||
| :aria-label="$t('common.scroll_to_top')" | ||
| @click="scrollToTop" | ||
| @click="scrollToTop()" |
There was a problem hiding this comment.
| @click="scrollToTop()" | |
| @click="()=>scrollToTop()" |
| class="fixed bottom-4 inset-ie-4 z-50 w-12 h-12 bg-bg-elevated border border-border rounded-full shadow-lg flex items-center justify-center text-fg-muted hover:text-fg transition-colors active:scale-95" | ||
| :aria-label="$t('common.scroll_to_top')" | ||
| @click="scrollToTop" | ||
| @click="scrollToTop()" |
There was a problem hiding this comment.
| @click="scrollToTop()" | |
| @click="()=>scrollToTop()" |
app/composables/useScrollToTop.ts
Outdated
| const cleanup = [ | ||
| useEventListener(window, 'wheel', cancel, { passive: true }), | ||
| useEventListener(window, 'touchstart', cancel, { passive: true }), | ||
| useEventListener(window, 'mousedown', cancel, { passive: true }), | ||
| ] |
There was a problem hiding this comment.
we should not add these inside of scrollTop(). This will lead to a memory leak, becuase the event listerners are not cleaned up after every call.
You should move them to the setup level.
app/composables/useScrollToTop.ts
Outdated
| rafId = requestAnimationFrame(animate) | ||
| } | ||
|
|
||
| onBeforeUnmount(cancel) |
There was a problem hiding this comment.
We should prefer the tryOnScopeDispose method from Vueuse. This is because the component might be called in another scope, than the component (for example as a sharedComposable).
@RYGRIT maybe we can switch to icon-only for the bottom bar on mobile? and long-press on the button shows what it does? |
Thank you very much for your suggestion. |
|
@danielroe Good call! I've updated the bottom bar to be icon-only on mobile (using sr-only to keep it a11y-friendly) and added title attributes. Regarding the long-press: I'd prefer to skip it for now. The icons are quite self-explanatory, and users rarely long-press for hints on mobile. Plus, long-pressing a link triggers the native browser context menu (like 'Open in new tab'), which we probably shouldn't override. Let me know what you think! |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/components/ScrollToTop.client.vue (2)
15-24: Reorder declarations to eliminate forward reference ofsupportsScrollStateQueries.
isVisible(lines 16–19) readssupportsScrollStateQueries.value, butsupportsScrollStateQueriesis only declared at line 20. Becausecomputed()callbacks are lazy this doesn't throw today, but any synchronous access toisVisible.valueinserted between those two declarations would hit the TDZ. DeclaresupportsScrollStateQueriesfirst.As a bonus,
if (supportsScrollStateQueries.value) return falseinsideisVisiblebecomes redundant once it's clear thatisVisibleis only consumed in thev-elsebranch (which only renders whensupportsScrollStateQueriesis already falsy) — you can drop that guard after reordering.♻️ Proposed reorder
const { y: scrollTop } = useScroll(window) -const isVisible = computed(() => { - if (supportsScrollStateQueries.value) return false - return scrollTop.value > SCROLL_TO_TOP_THRESHOLD -}) const { isSupported: supportsScrollStateQueries } = useCssSupports( 'container-type', 'scroll-state', { ssrValue: false }, ) +const isVisible = computed(() => scrollTop.value > SCROLL_TO_TOP_THRESHOLD)
35-35: Unnecessary lambda wrapper on@click.
@click="() => scrollToTop()"can be simplified to@click="scrollToTop". Same applies to line 55.♻️ Proposed simplification
- `@click`="() => scrollToTop()" + `@click`="scrollToTop"app/pages/package/[[org]]/[name].vue (1)
818-825: Same unnecessary lambda on@click.
@click="() => scrollToTop()"can be@click="scrollToTop"(same pattern as the comment onScrollToTop.client.vue).
4 participants
Fixes #1379
Improves the ScrollToTop component behavior.
scroll-statesupport.prefers-reduced-motion.Desktop
scroll-to-top.mp4
Mobile
freecompress-1714SVID_20260213_094933_1.mp4
cc @graphieros @MatteoGabriele