Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 13 additions & 10 deletions app/components/ScrollToTop.client.vue
Original file line number Diff line number Diff line change
Expand Up @@ -6,41 +6,44 @@ const excludedRoutes = new Set(['index', 'code'])

const isActive = computed(() => !excludedRoutes.has(route.name as string))

const SCROLL_TO_TOP_DURATION = 500

const isMounted = useMounted()
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)
Comment on lines +12 to +20
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok, requirements changed to only show the button on touch decices

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.


function onScroll() {
if (!supportsScrollStateQueries.value) {
if (supportsScrollStateQueries.value) {
return
}
isVisible.value = window.scrollY > scrollThreshold
}

function scrollToTop() {
window.scrollTo({ top: 0, behavior: 'smooth' })
}
const { scrollToTop } = useScrollToTop({ duration: SCROLL_TO_TOP_DURATION })

useEventListener('scroll', onScroll, { passive: true })

onMounted(() => {
isTouchDeviceClient.value = isTouchDevice()
onScroll()
})
</script>

<template>
<!-- When CSS scroll-state is supported, use CSS-only visibility -->
<button
v-if="isActive && supportsScrollStateQueries"
v-if="shouldShowButton && supportsScrollStateQueries"
type="button"
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 md:hidden flex items-center justify-center text-fg-muted hover:text-fg transition-colors active:scale-95"
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()"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@click="scrollToTop()"
@click="()=>scrollToTop()"

>
<span class="i-lucide:arrow-up w-5 h-5" aria-hidden="true" />
</button>
Expand All @@ -56,11 +59,11 @@ onMounted(() => {
leave-to-class="opacity-0 translate-y-2"
>
<button
v-if="isActive && isMounted && isVisible"
v-if="shouldShowButton && isMounted && isVisible"
type="button"
class="fixed bottom-4 inset-ie-4 z-50 w-12 h-12 bg-bg-elevated border border-border rounded-full shadow-lg md:hidden flex items-center justify-center text-fg-muted hover:text-fg transition-colors active:scale-95"
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()"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@click="scrollToTop()"
@click="()=>scrollToTop()"

>
<span class="i-lucide:arrow-up w-5 h-5" aria-hidden="true" />
</button>
Expand Down
96 changes: 96 additions & 0 deletions app/composables/useScrollToTop.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
interface UseScrollToTopOptions {
/**
* Duration of the scroll animation in milliseconds.
*/
duration?: number
}

/**
* Scroll to the top of the page with a smooth animation.
* @param options - Configuration options for the scroll animation.
* @returns An object containing the scrollToTop function and a cancel function.
*/
export function useScrollToTop(options: UseScrollToTopOptions) {
const { duration = 500 } = options

// Check if prefers-reduced-motion is enabled
const preferReducedMotion = useMediaQuery('(prefers-reduced-motion: reduce)')

// Easing function for the scroll animation
const easeOutQuad = (t: number) => t * (2 - t)

/**
* Active requestAnimationFrame id for the current auto-scroll animation
*/
let rafId: number | null = null
/**
* Disposer for temporary interaction listeners attached during auto-scroll
*/
let stopInteractionListeners: (() => void) | null = null

function cleanupInteractionListeners() {
if (stopInteractionListeners) {
stopInteractionListeners()
stopInteractionListeners = null
}
}

/**
* Stop any in-flight auto-scroll before starting a new one.
*/
function cancel() {
if (rafId !== null) {
cancelAnimationFrame(rafId)
rafId = null
}

cleanupInteractionListeners()
}

function scrollToTop() {
cancel()

if (preferReducedMotion.value) {
window.scrollTo({ top: 0, behavior: 'instant' })
return
}

const start = window.scrollY
if (start <= 0) return

const startTime = performance.now()
const change = -start

const cleanup = [
useEventListener(window, 'wheel', cancel, { passive: true }),
useEventListener(window, 'touchstart', cancel, { passive: true }),
useEventListener(window, 'mousedown', cancel, { passive: true }),
]
Comment on lines +64 to +68
Copy link
Contributor

@OrbisK OrbisK Feb 13, 2026

Choose a reason for hiding this comment

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

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.


stopInteractionListeners = () => cleanup.forEach(stop => stop())

// Start the frame-by-frame scroll animation.
function animate() {
const elapsed = performance.now() - startTime
const t = Math.min(elapsed / duration, 1)
const y = start + change * easeOutQuad(t)

window.scrollTo({ top: y })

if (t < 1) {
rafId = requestAnimationFrame(animate)
} else {
cancel()
}
}

rafId = requestAnimationFrame(animate)
}

onBeforeUnmount(cancel)
Copy link
Contributor

Choose a reason for hiding this comment

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

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).


return {
scrollToTop,
cancel,
}
}
Loading