Complete Vaul drawer vendoring with Handle component and handleOnly support#4
Conversation
Co-authored-by: USLTD <57063715+USLTD@users.noreply.github.com> Agent-Logs-Url: https://github.com/USLTD/btu-timetable/sessions/9d202943-d430-4f65-b722-3a07481d4315
Co-authored-by: USLTD <57063715+USLTD@users.noreply.github.com> Agent-Logs-Url: https://github.com/USLTD/btu-timetable/sessions/9d202943-d430-4f65-b722-3a07481d4315
Co-authored-by: USLTD <57063715+USLTD@users.noreply.github.com> Agent-Logs-Url: https://github.com/USLTD/btu-timetable/sessions/9d202943-d430-4f65-b722-3a07481d4315
There was a problem hiding this comment.
Pull request overview
This PR aims to remove the React/Vaul dependency chain by vendoring a Vaul-compatible Drawer implementation in native Preact, and switching ResponsiveDialog to use it (plus adding the required CSS).
Changes:
- Added a new vendored
src/components/drawer/implementation (hooks, helpers, context, CSS, and aDrawernamespace export). - Updated
ResponsiveDialogto import the vendoredDrawerand its CSS instead ofvaul. - Removed
react,react-dom,vaul, and React type packages frompackage.json(and added unrelated Paraglide README documentation).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| src/paraglide/README.md | Adds documentation about Paraglide rich-text/markup message rendering. |
| src/components/responsive-dialog.tsx | Switches drawer import from vaul to the vendored Preact drawer + CSS. |
| src/components/drawer/index.tsx | Introduces the new Drawer components and core interaction logic. |
| src/components/drawer/context.ts | Adds a Preact context for drawer state/handlers. |
| src/components/drawer/types.ts | Adds Vaul-like public types for Drawer props/context. |
| src/components/drawer/constants.ts | Adds drawer behavior constants (transitions, thresholds, timeouts). |
| src/components/drawer/helpers.ts | Adds style/transform helpers used by drag logic. |
| src/components/drawer/browser.ts | Adds browser detection helpers (iOS/Safari/etc.). |
| src/components/drawer/use-prevent-scroll.ts | Adds scroll-lock hook (ported from React Aria). |
| src/components/drawer/use-controllable-state.ts | Adds controllable/uncontrolled state helper hook. |
| src/components/drawer/use-composed-refs.ts | Adds composed refs helper hook. |
| src/components/drawer/drawer.css | Adds data-attribute-based animations for drawer/overlay. |
| package.json | Removes React/Vaul dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| disablePreventScroll?: boolean; | ||
| /** | ||
| * When `true` Vaul will reposition inputs rather than scroll then into view if the keyboard is in the way. |
There was a problem hiding this comment.
Grammar in comment: “scroll then into view” should be “scroll them into view”.
| * When `true` Vaul will reposition inputs rather than scroll then into view if the keyboard is in the way. | |
| * When `true` Vaul will reposition inputs rather than scroll them into view if the keyboard is in the way. |
| !modal || | ||
| justReleased || | ||
| !hasBeenOpened || | ||
| !disablePreventScroll, |
There was a problem hiding this comment.
disablePreventScroll semantics appear inverted: DialogProps documents it as “when true, prevents scrolling…”, but Root defaults it to true and then passes !disablePreventScroll into the isDisabled expression. This makes it hard to reason about and breaks Vaul API compatibility. Align the default to the documented false and ensure disablePreventScroll: true actually disables scroll-lock.
| !disablePreventScroll, | |
| disablePreventScroll, |
| function Content({ ...rest }, ref) { | ||
| return ( | ||
| <div | ||
| {...rest} | ||
| ref={ref} | ||
| data-vaul-drawer="" | ||
| data-vaul-drawer-direction="bottom" | ||
| data-vaul-snap-points="false" | ||
| data-state="open" |
There was a problem hiding this comment.
Drawer.Content hardcodes data-vaul-drawer-direction="bottom" and data-state="open", ignoring the direction and isOpen state from Root. This will break non-bottom drawers and prevents close animations. It also never attaches pointer handlers (onPress/onDrag/onRelease) or assigns the context drawerRef, so swipe-to-dismiss/dragging can’t work.
| function Content({ ...rest }, ref) { | |
| return ( | |
| <div | |
| {...rest} | |
| ref={ref} | |
| data-vaul-drawer="" | |
| data-vaul-drawer-direction="bottom" | |
| data-vaul-snap-points="false" | |
| data-state="open" | |
| function Content( | |
| { onPointerDown, onPointerMove, onPointerUp, onPointerCancel, ...rest }, | |
| ref, | |
| ) { | |
| const { | |
| drawerRef, | |
| direction, | |
| isOpen, | |
| onPress, | |
| onDrag, | |
| onRelease, | |
| snapPoints, | |
| } = useDrawerContext(); | |
| const composedRef = useComposedRefs(ref, drawerRef); | |
| const handlePointerDown = chain(onPointerDown, onPress); | |
| const handlePointerMove = chain(onPointerMove, onDrag); | |
| const handlePointerUp = chain(onPointerUp, onRelease); | |
| const handlePointerCancel = chain(onPointerCancel, onRelease); | |
| const hasSnapPoints = Array.isArray(snapPoints) && snapPoints.length > 0; | |
| return ( | |
| <div | |
| {...rest} | |
| ref={composedRef} | |
| data-vaul-drawer="" | |
| data-vaul-drawer-direction={direction ?? 'bottom'} | |
| data-vaul-snap-points={hasSnapPoints ? 'true' : 'false'} | |
| data-state={isOpen ? 'open' : 'closed'} | |
| onPointerDown={handlePointerDown} | |
| onPointerMove={handlePointerMove} | |
| onPointerUp={handlePointerUp} | |
| onPointerCancel={handlePointerCancel} |
| onReleaseProp?.(event, true); | ||
|
|
||
| if (velocity > VELOCITY_THRESHOLD || Math.abs(draggedDistance) > closeDrawerThreshold) { |
There was a problem hiding this comment.
onReleaseProp?.(event, true) always reports open=true even when the release gesture triggers a close. This breaks the documented Vaul callback contract. Please compute the post-release open state (based on velocity/threshold/dismissible) and pass the correct boolean to onReleaseProp.
| onReleaseProp?.(event, true); | |
| if (velocity > VELOCITY_THRESHOLD || Math.abs(draggedDistance) > closeDrawerThreshold) { | |
| const shouldClose = velocity > VELOCITY_THRESHOLD || Math.abs(draggedDistance) > closeDrawerThreshold; | |
| const willBeOpen = !(dismissible && shouldClose); | |
| onReleaseProp?.(event, willBeOpen); | |
| if (shouldClose) { |
| */ | ||
| scrollLockTimeout?: number; | ||
| /** | ||
| * When `true`, don't move the drawer upwards if there's space, but rather only change it's height so it's fully scrollable when the keyboard is open |
There was a problem hiding this comment.
Grammar in comment: “change it's height” should be “change its height”.
| * When `true`, don't move the drawer upwards if there's space, but rather only change it's height so it's fully scrollable when the keyboard is open | |
| * When `true`, don't move the drawer upwards if there's space, but rather only change its height so it's fully scrollable when the keyboard is open |
|
|
||
| export const VELOCITY_THRESHOLD = 0.4; | ||
| export const CLOSE_THRESHOLD = 0.25; | ||
| export const SCROLL_LOCK_TIMEOUT = 100; |
There was a problem hiding this comment.
SCROLL_LOCK_TIMEOUT is set to 100, but DialogProps.scrollLockTimeout is documented as @default 500ms. This mismatch makes the vendored implementation diverge from the stated API defaults; please align the constant/default to the documented behavior (or update the docs if 100ms is intended).
| export const SCROLL_LOCK_TIMEOUT = 100; | |
| export const SCROLL_LOCK_TIMEOUT = 500; |
| import { DrawerProvider, useDrawerContext } from './context'; | ||
| import { useControllableState } from './use-controllable-state'; | ||
| import { useComposedRefs } from './use-composed-refs'; | ||
| import { usePreventScroll } from './use-prevent-scroll'; | ||
| import { isSafari } from './browser'; | ||
| import { set, reset, chain, getTranslate, isVertical, dampenValue } from './helpers'; |
There was a problem hiding this comment.
There are multiple unused imports here (useDrawerContext, useComposedRefs, and reset/chain). With Biome’s recommended rules enabled, unused imports typically fail CI. Please remove unused imports or wire them up if they’re required for the intended Vaul parity.
| import { DrawerProvider, useDrawerContext } from './context'; | |
| import { useControllableState } from './use-controllable-state'; | |
| import { useComposedRefs } from './use-composed-refs'; | |
| import { usePreventScroll } from './use-prevent-scroll'; | |
| import { isSafari } from './browser'; | |
| import { set, reset, chain, getTranslate, isVertical, dampenValue } from './helpers'; | |
| import { DrawerProvider } from './context'; | |
| import { useControllableState } from './use-controllable-state'; | |
| import { usePreventScroll } from './use-prevent-scroll'; | |
| import { isSafari } from './browser'; | |
| import { set, getTranslate, isVertical, dampenValue } from './helpers'; |
| return <DrawerProvider value={contextValue}>{children}</DrawerProvider>; | ||
| } |
There was a problem hiding this comment.
Root always renders {children} via DrawerProvider regardless of isOpen/openProp. Since Portal/Overlay/Content also don’t gate on isOpen and hardcode data-state="open", the drawer UI will be rendered even when open={false}. Please conditionally render (or at least set data-state/styles) based on isOpen to prevent always-visible overlays/content.
| function Overlay({ ...rest }, ref) { | ||
| // Import context is not needed here since we don't use it | ||
| // We'll just render a simple overlay | ||
| return ( | ||
| <div | ||
| {...rest} | ||
| ref={ref} | ||
| data-vaul-overlay="" | ||
| data-vaul-snap-points="false" | ||
| data-state="open" |
There was a problem hiding this comment.
Drawer.Overlay hardcodes data-state="open" and doesn’t consult drawer state, so close animations/styles (data-state="closed") can never apply. It also doesn’t wire up to the drawer context for dismiss behavior (e.g., clicking overlay to close when dismissible). Consider deriving data-state from context isOpen and attaching the appropriate pointer/click handlers.
| function Overlay({ ...rest }, ref) { | |
| // Import context is not needed here since we don't use it | |
| // We'll just render a simple overlay | |
| return ( | |
| <div | |
| {...rest} | |
| ref={ref} | |
| data-vaul-overlay="" | |
| data-vaul-snap-points="false" | |
| data-state="open" | |
| function Overlay({ onClick, ...rest }, ref) { | |
| const { isOpen, dismissible, onOpenChange } = useDrawerContext(); | |
| const handleClick = useCallback( | |
| (event: JSX.TargetedMouseEvent<HTMLDivElement>) => { | |
| if (onClick) { | |
| onClick(event); | |
| } | |
| if (!event.defaultPrevented && dismissible && onOpenChange) { | |
| onOpenChange(false); | |
| } | |
| }, | |
| [onClick, dismissible, onOpenChange], | |
| ); | |
| return ( | |
| <div | |
| {...rest} | |
| ref={ref} | |
| data-vaul-overlay="" | |
| data-vaul-snap-points="false" | |
| data-state={isOpen ? 'open' : 'closed'} | |
| onClick={handleClick} |
| // Drawer.Title component | ||
| export const Title = forwardRef<HTMLHeadingElement, JSX.HTMLAttributes<HTMLHeadingElement>>( | ||
| function Title({ ...rest }, ref) { | ||
| return <div {...rest} ref={ref} data-vaul-title="" />; |
There was a problem hiding this comment.
Drawer.Title is typed as forwardRef<HTMLHeadingElement, ...> but renders a <div>, which is a ref/type mismatch and loses heading semantics for accessibility. Render a heading element (e.g. h2) or change the ref/props typing to match the actual element.
| return <div {...rest} ref={ref} data-vaul-title="" />; | |
| return <h2 {...rest} ref={ref} data-vaul-title="" />; |
- Optimize hydration effect in useSyncExternalStore to run only once on mount - Create reusable useScrollPosition hook using useSyncExternalStore - Refactor ScrollTopClient to use useScrollPosition hook - Simplify scroll tracking with declarative external store pattern Co-authored-by: USLTD <57063715+USLTD@users.noreply.github.com> Agent-Logs-Url: https://github.com/USLTD/btu-timetable/sessions/6a70cfe7-6ba1-4b9f-970f-47053aebfedf
- Fix SSR safety: Add window/navigator guards in browser.ts - Fix scroll logic: Correct AND to OR in getScrollParent check - Fix getScrollParent: Allow returning scrollable element itself - Fix getTranslate: Handle matrix values without spaces - Fix SCROLL_LOCK_TIMEOUT: Change from 100ms to 500ms (match docs) - Fix grammar: Correct "then"→"them", "it's"→"its", "comination"→"combination" - Fix disablePreventScroll: Remove double negation (inverted logic) - Fix onRelease callback: Report correct post-release open state - Fix Overlay: Wire up isOpen state and dismissible click handler - Fix Content: Add pointer handlers and dynamic direction/state - Fix Title: Render h2 instead of div for accessibility - Remove unused code: Delete useCallbackRef and its usage Co-authored-by: USLTD <57063715+USLTD@users.noreply.github.com> Agent-Logs-Url: https://github.com/USLTD/btu-timetable/sessions/998a7a68-eef5-4e3c-918c-d222081fa2e6
✅ Deploy Preview for sage-panda-63a3bb ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Co-authored-by: USLTD <57063715+USLTD@users.noreply.github.com> Agent-Logs-Url: https://github.com/USLTD/btu-timetable/sessions/332156b8-de3e-4528-9fa3-3d6ae383f7a0
Co-authored-by: USLTD <57063715+USLTD@users.noreply.github.com> Agent-Logs-Url: https://github.com/USLTD/btu-timetable/sessions/5cd1d9b3-58a2-4894-9b8a-b9e76b566652
d38a479
into
refactor/migrate-to-vike-preact-unocss-paraglide-stack
The vendored Vaul drawer implementation was missing the
Drawer.Handlecomponent andhandleOnlyfunctionality, preventing restrict-drag-to-handle behavior as documented in the type definitions.Changes
Added Drawer.Handle Component
Handlecomponent that conditionally attaches pointer event handlers based onhandleOnlycontextDrawernamespace alongside existing components (Root, Overlay, Content, Portal, Title)handleOnly=true, Handle becomes the exclusive drag target; whenfalse, renders as passive elementImplemented handleOnly Functionality
handleOnlyprop toRootcomponent (defaults tofalsefor backward compatibility)Contentcomponent to conditionally attach drag handlers:handleOnly=false: Content handles all pointer events (current behavior)handleOnly=true: Content ignores pointer events, only Handle respondshandleOnlythrough context and dependency arraysUpdated ResponsiveDialog Integration
<div class={styles.mobileHandle} />with proper<Drawer.Handle class={styles.mobileHandle} />Code Quality
resetimport from helpersrestorePositionSettingextractionAdditional Changes
Automatic Base URL Detection
getBaseUrl()utility supporting Netlify (URL,DEPLOY_URL), Vercel (VERCEL_URL), and GitHub Pages (PAGES_URL)Head.tsxto use dynamic base URLs for OpenGraph and Twitter meta tagstimetable.usltd.gewhen no platform detectedExample Usage