Drawer POC #1145
Drawer POC #1145
Conversation
WalkthroughA comprehensive Drawer UI component system has been introduced, including context, root, trigger, overlay, portal, content, title, description, and close subcomponents. The Drawer supports advanced features such as nested drawers, drag-to-close, background scaling, and customizable animations. Extensive Storybook stories demonstrate its various configurations and interactive behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DrawerTrigger
participant DrawerRoot
participant DrawerContext
participant DrawerOverlay
participant DrawerContent
participant DrawerClose
User->>DrawerTrigger: Clicks trigger
DrawerTrigger->>DrawerRoot: Finds nearest DrawerRoot and sets open=true
DrawerRoot->>DrawerContext: Provides context (open state, handlers)
DrawerRoot->>DrawerOverlay: Renders overlay (if open)
DrawerRoot->>DrawerContent: Renders content (if open)
DrawerContent->>User: Handles drag gestures (optional)
DrawerOverlay->>User: Handles overlay clicks (to close)
DrawerContent->>DrawerClose: Renders close button
DrawerClose->>DrawerRoot: Triggers close on click
DrawerRoot->>DrawerContext: Updates context (open=false)
DrawerOverlay->>DrawerRoot: Unmounts overlay
DrawerContent->>DrawerRoot: Unmounts content
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (10)
src/components/ui/Drawer/Drawer.tsx (1)
12-15: Consider alternative approaches for compound component pattern.The warning-based approach works but is somewhat unusual. Consider these alternatives:
- Non-callable object: Export an object instead of a function
- Throw error: Throw an error instead of just warning
Alternative 1: Non-callable object
-const Drawer = () => { - console.warn('Direct usage of Drawer is not supported. Please use Drawer.Root, Drawer.Content, etc. instead.'); - return null; -}; +const Drawer = {} as { + Root: typeof DrawerRoot; + Trigger: typeof DrawerTrigger; + Portal: typeof DrawerPortal; + Overlay: typeof DrawerOverlay; + Content: typeof DrawerContent; + Title: typeof DrawerTitle; + Description: typeof DrawerDescription; + Close: typeof DrawerClose; +};Alternative 2: Throw error
- console.warn('Direct usage of Drawer is not supported. Please use Drawer.Root, Drawer.Content, etc. instead.'); - return null; + throw new Error('Direct usage of Drawer is not supported. Please use Drawer.Root, Drawer.Content, etc. instead.');src/components/ui/Drawer/fragments/DrawerTitle.tsx (1)
5-5: Remove unused import.
DialogPrimitiveis imported but never used in this component.-import DialogPrimitive from '~/core/primitives/Dialog';src/components/ui/Drawer/context/DrawerContext.tsx (1)
36-36: Simplify conditional check with optional chaining.The chained property checks can be simplified using optional chaining as suggested by static analysis.
- if (parentContext && parentContext.rootClass && parentContext.rootClass !== '') { + if (parentContext?.rootClass !== '') {🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/ui/Drawer/fragments/DrawerRoot.tsx (1)
167-171: Consider alternative to DOM property attachment.Directly attaching properties to DOM elements can be fragile and is generally discouraged. Consider using a ref callback or data attributes instead.
- ref={(el) => { - if (el) { - (el as any).__drawerOpenChange = handleOpenChange; - } - }} + ref={(el) => { + if (el) { + el.setAttribute('data-drawer-open-change', 'true'); + } + }}Or better yet, consider if this property is actually needed - it may be an artifact from development.
src/components/ui/Drawer/fragments/DrawerOverlay.tsx (1)
6-6: Remove unused import.
DialogPrimitiveis imported but never used in this component.-import DialogPrimitive from '~/core/primitives/Dialog';🧰 Tools
🪛 GitHub Check: lint
[warning] 6-6:
'DialogPrimitive' is defined but never usedsrc/components/ui/Drawer/fragments/DrawerTrigger.tsx (1)
19-38: Consider using React patterns instead of DOM traversal.The current implementation relies on DOM traversal and attaching properties to DOM elements, which is fragile and goes against React's declarative paradigm. This approach could break with React updates or when the DOM structure changes.
Consider these alternatives:
- Use a dedicated context for each drawer instance
- Pass a ref callback through props
- Use a state management solution
Example using ref callback:
// In parent component const triggerRef = useRef<{ open: () => void }>(null); // Pass to trigger <DrawerTrigger ref={triggerRef} />src/components/ui/Drawer/fragments/DrawerContent.tsx (1)
222-231: Remove redundant case clause.The
case 'bottom':is redundant since it has the same implementation as the default case.-case 'bottom': default: return { ...baseStyles, bottom: 0, left: 0, right: 0, transform: `translate3d(0, ${translatePercent}%, 0)` };🧰 Tools
🪛 Biome (1.9.4)
[error] 222-222: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
src/components/ui/Drawer/stories/Drawer.stories.tsx (3)
269-275: Remove or uncomment the feature list.This commented code appears to be a feature list that should either be displayed or removed entirely.
-{/* <ul style={{ paddingLeft: '20px', lineHeight: '1.8' }}> - <li>Background content scales to 95% size</li> - <li>Rounded corners applied to the entire page</li> - <li>Smooth transitions synchronized with drawer animation</li> - <li>Configurable scale amount and border radius</li> - <li>Automatic cleanup on drawer close</li> -</ul> */} +<ul style={{ paddingLeft: '20px', lineHeight: '1.8' }}> + <li>Background content scales to 95% size</li> + <li>Rounded corners applied to the entire page</li> + <li>Smooth transitions synchronized with drawer animation</li> + <li>Configurable scale amount and border radius</li> + <li>Automatic cleanup on drawer close</li> +</ul>
318-320: Remove empty Drawer.Description component.The empty Description component adds unnecessary DOM elements.
</Drawer.Title> -<Drawer.Description> - -</Drawer.Description> <div style={{ display: 'flex', gap: '12px', justifyContent: 'flex-end' }}>
1-725: Consider extracting common styles for better maintainability.The stories use extensive inline styles which makes them hard to maintain and creates repetition. Consider extracting common styles into constants or using CSS modules/styled-components.
Example refactor:
const drawerStyles = { content: { padding: '20px', backgroundColor: 'white', borderRadius: '12px', }, title: { margin: '0 0 8px 0', fontSize: '18px', fontWeight: 'bold', }, description: { margin: '0 0 16px 0', color: '#666', }, } as const; // Usage <div style={drawerStyles.content}> <Drawer.Title> <h2 style={drawerStyles.title}>Title</h2> </Drawer.Title> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/ui/Drawer/Drawer.tsx(1 hunks)src/components/ui/Drawer/context/DrawerContext.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerClose.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerContent.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerDescription.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerOverlay.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerPortal.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerRoot.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerTitle.tsx(1 hunks)src/components/ui/Drawer/fragments/DrawerTrigger.tsx(1 hunks)src/components/ui/Drawer/stories/Drawer.stories.tsx(1 hunks)src/core/hooks/useIsInsideFrom/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/ui/Drawer/fragments/DrawerDescription.tsx (1)
src/components/ui/Drawer/context/DrawerContext.tsx (1)
DrawerContext(17-29)
🪛 GitHub Check: lint
src/components/ui/Drawer/fragments/DrawerOverlay.tsx
[warning] 66-66:
Visible, non-interactive elements with click handlers must have at least one keyboard listener
[warning] 6-6:
'DialogPrimitive' is defined but never used
src/components/ui/Drawer/fragments/DrawerContent.tsx
[warning] 240-240:
Non-interactive elements should not be assigned mouse or keyboard event listeners
[warning] 240-240:
Visible, non-interactive elements with click handlers must have at least one keyboard listener
🪛 Biome (1.9.4)
src/components/ui/Drawer/context/DrawerContext.tsx
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/ui/Drawer/fragments/DrawerContent.tsx
[error] 222-222: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (15)
src/core/hooks/useIsInsideFrom/index.ts (5)
9-9: No-op formatting change
Pure indentation adjustment on theuseStateinitialization; logic remains unchanged.
11-15: No-op formatting change
Indentation tweaks within theuseEffectearly-return block; behavior is identical.
17-24: No-op formatting change
Adjusted indentation in the ancestor-traversal loop; no functional impact.
26-27: No-op formatting change
Indentation refinement for the final state update and dependency array; logic unchanged.
29-29: No-op formatting change
Indentation normalized on the hook’s return statement; functionality intact.src/components/ui/Drawer/fragments/DrawerClose.tsx (1)
13-25: LGTM! Clean implementation with proper context usage.The component correctly consumes the DrawerContext, handles className composition with clsx, and properly forwards props to the underlying DialogPrimitive.Action.
src/components/ui/Drawer/Drawer.tsx (1)
17-24: LGTM! Clean compound component API.The static property assignments create a clean, discoverable API for the drawer system. This pattern enables intuitive usage like
Drawer.Root,Drawer.Content, etc.src/components/ui/Drawer/fragments/DrawerTitle.tsx (1)
13-31: Component implementation looks solid.The asChild pattern is correctly implemented with proper fallbacks and prop merging. The use of React.cloneElement with id fallback ensures accessibility while maintaining flexibility.
src/components/ui/Drawer/context/DrawerContext.tsx (2)
17-29: Well-structured context with sensible defaults.The context provides a comprehensive API for drawer state management with appropriate default values for all properties.
32-52: Robust nesting hook with proper error handling.The useDrawerNesting hook correctly handles parent context detection and provides sensible fallbacks with error handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/ui/Drawer/fragments/DrawerRoot.tsx (2)
27-52: Excellent state management implementation.The component properly handles both controlled and uncontrolled modes with appropriate synchronization between open state and drag progress.
60-126: Sophisticated background scaling with proper cleanup.The background scaling effect is well-implemented with proper nesting support, smooth transitions, and thorough cleanup on unmount.
src/components/ui/Drawer/fragments/DrawerOverlay.tsx (2)
20-58: Excellent animation timing implementation.The use of double requestAnimationFrame for smooth animation start and proper cleanup of timeouts/RAF is well-implemented.
74-84: Smart performance optimizations.The inline styles with
willChange,backfaceVisibility, andtranslateZ(0)provide good performance optimizations for animations.src/components/ui/Drawer/fragments/DrawerContent.tsx (1)
19-50: Well-implemented animation state management!The animation logic with double RAF for entering animations and proper cleanup is well thought out. The state transitions are clear and the timing synchronization with the overlay is a nice touch.
| export type DrawerPortalProps = { | ||
| children: React.ReactNode; | ||
| } |
There was a problem hiding this comment.
Fix type definition to include additional props.
The DrawerPortalProps type only defines children, but the component implementation accepts additional props via rest parameters (...props). This creates a type safety gap.
-export type DrawerPortalProps = {
- children: React.ReactNode;
-}
+export type DrawerPortalProps = React.ComponentProps<typeof DialogPrimitive.Portal> & {
+ children: React.ReactNode;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type DrawerPortalProps = { | |
| children: React.ReactNode; | |
| } | |
| export type DrawerPortalProps = React.ComponentProps<typeof DialogPrimitive.Portal> & { | |
| children: React.ReactNode; | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerPortal.tsx around lines 5 to 7, the
DrawerPortalProps type only includes the children prop but the component also
accepts additional props via rest parameters. Update the DrawerPortalProps type
to extend React.HTMLAttributes or a suitable interface to include these
additional props, ensuring type safety for all passed properties.
| return ( | ||
| <Comp {...childProps} id="drawer-description"> | ||
| {children} | ||
| </Comp> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify non-asChild rendering logic.
Since the asChild case is now handled separately, the remaining logic can be simplified.
- return (
- <Comp {...childProps} id="drawer-description">
- {children}
- </Comp>
- );
+ return (
+ <p className={clsx(`${rootClass}-description`, className)} {...props} id="drawer-description">
+ {children}
+ </p>
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <Comp {...childProps} id="drawer-description"> | |
| {children} | |
| </Comp> | |
| ); | |
| return ( | |
| <p | |
| className={clsx(`${rootClass}-description`, className)} | |
| {...props} | |
| id="drawer-description" | |
| > | |
| {children} | |
| </p> | |
| ); |
🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerDescription.tsx around lines 25 to
29, simplify the rendering logic by removing any conditional checks for the
asChild prop, since that case is handled separately. Directly return the
component with the passed childProps and children without extra branching or
conditions.
| const Comp = asChild ? React.Fragment : 'p'; | ||
| const childProps = asChild ? {} : { className: clsx(`${rootClass}-description`, className), ...props }; | ||
|
|
||
| if (asChild && React.isValidElement(children)) { | ||
| return React.cloneElement(children, { | ||
| ...childProps, | ||
| ...children.props, | ||
| id: children.props.id || 'drawer-description' | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix asChild implementation logic.
The current asChild implementation has several issues:
- When
asChildis true,childPropsis set to an empty object (line 15), so the context-based className is never applied to cloned elements. - The cloned element doesn't receive the properly composed className from the context.
- const Comp = asChild ? React.Fragment : 'p';
- const childProps = asChild ? {} : { className: clsx(`${rootClass}-description`, className), ...props };
-
- if (asChild && React.isValidElement(children)) {
- return React.cloneElement(children, {
- ...childProps,
- ...children.props,
- id: children.props.id || 'drawer-description'
- });
- }
+ if (asChild && React.isValidElement(children)) {
+ return React.cloneElement(children, {
+ ...props,
+ ...children.props,
+ className: clsx(`${rootClass}-description`, className, children.props.className),
+ id: children.props.id || 'drawer-description'
+ });
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerDescription.tsx around lines 14 to
23, the asChild logic sets childProps to an empty object when asChild is true,
causing the context-based className to be omitted from cloned elements. To fix
this, ensure childProps always includes the composed className regardless of
asChild's value, so when cloning children, the className from context and any
additional props are properly merged and applied.
| <div | ||
| className={clsx(`${rootClass}-overlay`, className)} | ||
| onClick={(event) => { | ||
| // Only close if clicking directly on the overlay, not on nested content | ||
| if (event.target === event.currentTarget) { | ||
| handleOverlayClick(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard accessibility for overlay dismissal.
The overlay should be dismissible via keyboard (ESC key) to meet accessibility standards.
- onClick={(event) => {
+ onClick={(event) => {
// Only close if clicking directly on the overlay, not on nested content
if (event.target === event.currentTarget) {
handleOverlayClick();
}
}}
+ onKeyDown={(event) => {
+ if (event.key === 'Escape') {
+ handleOverlayClick();
+ }
+ }}
+ tabIndex={-1}🧰 Tools
🪛 GitHub Check: lint
[warning] 66-66:
Visible, non-interactive elements with click handlers must have at least one keyboard listener
🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerOverlay.tsx around lines 66 to 73,
the overlay div currently only supports dismissal via mouse click. To improve
accessibility, add a keyboard event listener that listens for the ESC key press
and triggers the handleOverlayClick function to dismiss the overlay. Ensure the
div is focusable by adding a tabIndex attribute if necessary, and handle the
keydown event to detect the ESC key.
| return React.cloneElement(children, { | ||
| ...children.props, | ||
| className: clsx(`${rootClass}-trigger`, className, children.props.className), | ||
| onClick: handleClick | ||
| }); |
There was a problem hiding this comment.
Fix props spreading order to prevent unintended overrides.
The current props spreading order allows passed props to override the component's onClick handler and className, which could break functionality.
-return React.cloneElement(children, {
- ...children.props,
- className: clsx(`${rootClass}-trigger`, className, children.props.className),
- onClick: handleClick
-});
+return React.cloneElement(children, {
+ ...children.props,
+ className: clsx(`${rootClass}-trigger`, className, children.props.className),
+ onClick: (e: React.MouseEvent) => {
+ children.props.onClick?.(e);
+ handleClick(e);
+ }
+});Similarly for ButtonPrimitive:
<ButtonPrimitive
+ {...props}
className={clsx(`${rootClass}-trigger`, className)}
onClick={handleClick}
- {...props}
>Also applies to: 50-53
🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerTrigger.tsx around lines 42 to 46
and 50 to 53, the order of props spreading causes passed props to override the
component's onClick handler and className, potentially breaking functionality.
To fix this, reorder the spread so that children.props and passed props come
before the explicitly set className and onClick, ensuring the component's
handlers and className take precedence and are not unintentionally overridden.
| let element = event.currentTarget as HTMLElement; | ||
| while (element) { | ||
| if (element.hasAttribute('data-drawer-root')) { | ||
| const openChangeHandler = (element as any).__drawerOpenChange; |
There was a problem hiding this comment.
Replace unsafe type casting with proper type definition.
Using as any defeats TypeScript's type safety. Consider defining a proper interface for the extended HTMLElement.
-const openChangeHandler = (element as any).__drawerOpenChange;
+interface DrawerElement extends HTMLElement {
+ __drawerOpenChange?: (open: boolean) => void;
+}
+const openChangeHandler = (element as DrawerElement).__drawerOpenChange;Additionally, consider using a WeakMap or data attribute approach instead of directly attaching properties to DOM elements for better encapsulation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const openChangeHandler = (element as any).__drawerOpenChange; | |
| interface DrawerElement extends HTMLElement { | |
| __drawerOpenChange?: (open: boolean) => void; | |
| } | |
| const openChangeHandler = (element as DrawerElement).__drawerOpenChange; |
🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerTrigger.tsx at line 30, replace the
unsafe type casting using 'as any' by defining a proper TypeScript interface
that extends HTMLElement to include the __drawerOpenChange property. Then use
this interface for the element's type. Additionally, refactor the code to avoid
attaching properties directly to DOM elements by using a WeakMap or data
attributes to store and access the __drawerOpenChange handler, improving
encapsulation and type safety.
| dragThresholdRef.current = true; | ||
| setIsDragging(true); | ||
| } | ||
|
|
||
| // Only process drag if we've exceeded threshold | ||
| if (!dragThresholdRef.current) return; | ||
|
|
||
| let progress = initialProgressRef.current; // Start from initial progress | ||
|
|
||
| switch (side) { | ||
| case 'bottom': { | ||
| // Dragging up closes, dragging down opens | ||
| const bottomMovement = -deltaY / window.innerHeight; | ||
| progress = Math.max(0, Math.min(1, initialProgressRef.current + bottomMovement)); | ||
| break; | ||
| } | ||
| case 'top': { | ||
| // Dragging down closes, dragging up opens | ||
| const topMovement = deltaY / window.innerHeight; | ||
| progress = Math.max(0, Math.min(1, initialProgressRef.current + topMovement)); | ||
| break; | ||
| } | ||
| case 'right': { | ||
| // Dragging left closes, dragging right opens | ||
| const rightMovement = -deltaX / window.innerWidth; | ||
| progress = Math.max(0, Math.min(1, initialProgressRef.current + rightMovement)); | ||
| break; | ||
| } | ||
| case 'left': { | ||
| // Dragging right closes, dragging left opens | ||
| const leftMovement = deltaX / window.innerWidth; | ||
| progress = Math.max(0, Math.min(1, initialProgressRef.current + leftMovement)); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| handleDragProgress(progress); | ||
| }; | ||
|
|
||
| const handleLocalDragEnd = () => { | ||
| // Only handle drag end if we actually started dragging | ||
| if (dragThresholdRef.current) { | ||
| setIsDragging(false); | ||
| // Snap to open or closed based on current progress | ||
| const finalProgress = dragProgress > 0.5 ? 1 : 0; | ||
| handleDragEnd(finalProgress); | ||
| } | ||
| dragThresholdRef.current = false; | ||
| }; | ||
|
|
||
| // Mouse events | ||
| const handleMouseDown = (e: React.MouseEvent) => { | ||
| // Don't prevent default immediately - let normal clicks work | ||
| const startY = e.clientY; | ||
| const startX = e.clientX; | ||
| handleDragStart(startY, startX); | ||
|
|
||
| const handleMouseMove = (e: MouseEvent) => { | ||
| // Prevent default once we start dragging | ||
| if (dragThresholdRef.current) { | ||
| e.preventDefault(); | ||
| } | ||
| handleDragMove(e.clientY, e.clientX, startY, startX); | ||
| }; | ||
|
|
||
| const handleMouseUp = () => { | ||
| handleLocalDragEnd(); | ||
| document.removeEventListener('mousemove', handleMouseMove); | ||
| document.removeEventListener('mouseup', handleMouseUp); | ||
| }; | ||
|
|
||
| document.addEventListener('mousemove', handleMouseMove); | ||
| document.addEventListener('mouseup', handleMouseUp); | ||
| }; | ||
|
|
||
| // Touch events | ||
| const handleTouchStart = (e: React.TouchEvent) => { | ||
| const touch = e.touches[0]; | ||
| const startY = touch.clientY; | ||
| const startX = touch.clientX; | ||
| handleDragStart(startY, startX); | ||
|
|
||
| const handleTouchMove = (e: TouchEvent) => { | ||
| // Only prevent default once we start dragging | ||
| if (dragThresholdRef.current) { | ||
| e.preventDefault(); | ||
| } | ||
| const touch = e.touches[0]; | ||
| handleDragMove(touch.clientY, touch.clientX, startY, startX); | ||
| }; | ||
|
|
||
| const handleTouchEnd = () => { | ||
| handleLocalDragEnd(); | ||
| document.removeEventListener('touchmove', handleTouchMove); | ||
| document.removeEventListener('touchend', handleTouchEnd); | ||
| }; | ||
|
|
||
| document.addEventListener('touchmove', handleTouchMove, { passive: false }); | ||
| document.addEventListener('touchend', handleTouchEnd); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract drag logic into a custom hook for better maintainability.
The drag functionality spans over 100 lines and makes the component complex. Consider extracting it into a reusable hook.
Create a custom hook:
// useDraggableDrawer.ts
export const useDraggableDrawer = ({
side,
dragProgress,
handleDragProgress,
handleDragEnd,
}: UseDraggableDrawerProps) => {
const [isDragging, setIsDragging] = useState(false);
const initialProgressRef = useRef(0);
const dragThresholdRef = useRef(false);
// ... move all drag logic here ...
return {
isDragging,
handleMouseDown,
handleTouchStart,
};
};Then simplify the component:
const { isDragging, handleMouseDown, handleTouchStart } = useDraggableDrawer({
side,
dragProgress,
handleDragProgress,
handleDragEnd,
});🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerContent.tsx around lines 52 to 171,
the drag functionality code is lengthy and complex within the component. Extract
all drag-related state, refs, and handlers (handleDragStart, handleDragMove,
handleLocalDragEnd, handleMouseDown, handleTouchStart, and their inner event
handlers) into a new custom hook named useDraggableDrawer. This hook should
accept side, dragProgress, handleDragProgress, and handleDragEnd as parameters,
manage isDragging state internally, and return isDragging along with
handleMouseDown and handleTouchStart functions. Then, replace the existing drag
logic in the component with calls to this hook to simplify and improve
maintainability.
| <div | ||
| ref={containerRef} | ||
| style={getDrawerStyles()} | ||
| className={clsx(`${rootClass}-content`, className)} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| data-state={isOpen ? 'open' : 'closed'} | ||
| data-side={side} | ||
| onMouseDown={handleMouseDown} | ||
| onTouchStart={handleTouchStart} | ||
| onClick={(event) => { | ||
| // Prevent clicks on content from propagating to overlay | ||
| event.stopPropagation(); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard support and improve accessibility.
The drawer content has mouse/touch handlers but lacks keyboard support. Additionally, the non-interactive div with event handlers raises accessibility concerns.
Consider these improvements:
- Add keyboard event handlers for arrow keys to enable keyboard-based dragging
- Add appropriate ARIA attributes for draggable content
- Make the element focusable when draggable
<div
ref={containerRef}
style={getDrawerStyles()}
className={clsx(`${rootClass}-content`, className)}
role="dialog"
aria-modal="true"
+ aria-grabbed={isDragging}
+ tabIndex={0}
data-state={isOpen ? 'open' : 'closed'}
data-side={side}
onMouseDown={handleMouseDown}
onTouchStart={handleTouchStart}
+ onKeyDown={handleKeyDown}
onClick={(event) => {
// Prevent clicks on content from propagating to overlay
event.stopPropagation();
}}
>Add keyboard handler:
const handleKeyDown = (e: React.KeyboardEvent) => {
if (!isOpen) return;
switch (e.key) {
case 'ArrowUp':
case 'ArrowDown':
case 'ArrowLeft':
case 'ArrowRight':
e.preventDefault();
// Implement keyboard-based drag logic
break;
case 'Escape':
handleDragEnd(0);
break;
}
};🧰 Tools
🪛 GitHub Check: lint
[warning] 240-240:
Non-interactive elements should not be assigned mouse or keyboard event listeners
[warning] 240-240:
Visible, non-interactive elements with click handlers must have at least one keyboard listener
🤖 Prompt for AI Agents
In src/components/ui/Drawer/fragments/DrawerContent.tsx around lines 240 to 253,
the drawer content div has mouse and touch event handlers but lacks keyboard
support and accessibility features. To fix this, add a keyboard event handler
for arrow keys and Escape to enable keyboard-based dragging and closing. Make
the div focusable by adding a tabIndex attribute when draggable, and include
appropriate ARIA attributes such as aria-grabbed or aria-dropeffect to indicate
draggable content. Attach the new handleKeyDown function to the div's onKeyDown
event to handle keyboard interactions.
| import Button from '~/components/ui/Button/Button'; | ||
|
|
||
| const CloseIcon = () => { | ||
| return <svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M12.8536 2.85355C13.0488 2.65829 13.0488 2.34171 12.8536 2.14645C12.6583 1.95118 12.3417 1.95118 12.1464 2.14645L7.5 6.79289L2.85355 2.14645C2.65829 1.95118 2.34171 1.95118 2.14645 2.14645C1.95118 2.34171 1.95118 2.65829 2.14645 2.85355L6.79289 7.5L2.14645 12.1464C1.95118 12.3417 1.95118 12.6583 2.14645 12.8536C2.34171 13.0488 2.65829 13.0488 2.85355 12.8536L7.5 8.20711L12.1464 12.8536C12.3417 13.0488 12.6583 13.0488 12.8536 12.8536C13.0488 12.6583 13.0488 12.3417 12.8536 12.1464L8.20711 7.5L12.8536 2.85355Z" fill="currentColor" fill-rule="evenodd" clip-rule="evenodd"></path></svg>; |
There was a problem hiding this comment.
Use React's camelCase naming for SVG attributes.
React requires camelCase for SVG attributes.
-<path d="..." fill="currentColor" fill-rule="evenodd" clip-rule="evenodd"></path>
+<path d="..." fill="currentColor" fillRule="evenodd" clipRule="evenodd"></path>🤖 Prompt for AI Agents
In src/components/ui/Drawer/stories/Drawer.stories.tsx at line 8, the SVG
attributes use hyphenated names like fill-rule and clip-rule, which are not
compliant with React's camelCase naming convention. Update these attribute names
to camelCase equivalents: change fill-rule to fillRule and clip-rule to clipRule
to ensure proper rendering in React.
Summary by CodeRabbit
New Features
Style