Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughA new composable Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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()" |
| 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.
| 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? |
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