Feat/add draggable modal #3983
Feat/add draggable modal #3983
Conversation
Co-authored-by: Ryo Matsukawa <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: bf4affb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a draggable modal component to the project, fulfilling the feature request outlined in issue #2647. It integrates two new dependencies, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (17)
packages/components/modal/src/index.ts (1)
18-18: Consider documenting the new export.The
useDraggablehook is a significant addition that enables the new draggable modal feature. Consider adding a JSDoc comment to document its purpose and usage.+/** + * Hook for adding drag functionality to modals. + * @see Issue #2647 for more details about the draggable modal feature. + */ export {useDraggable} from "@nextui-org/use-draggable";packages/components/modal/src/modal-header.tsx (1)
8-13: Enhance ref prop documentationThe interface changes look good and maintain backward compatibility. However, the JSDoc could be more descriptive about the ref's purpose.
Consider updating the JSDoc to be more descriptive:
/** - * Ref to the DOM node. + * Ref to the DOM node. Used internally for implementing draggable behavior + * when the modal is configured to be draggable. */packages/hooks/use-draggable/package.json (1)
44-51: Consider updating the build targetThe current ES2019 target might be unnecessarily conservative. Consider targeting a more recent ECMAScript version (e.g., ES2021) to leverage modern JavaScript features, as the project already requires React 18+.
apps/docs/content/components/modal/draggable.ts (1)
39-45: Add TypeScript types for better maintainabilityConsider adding type definitions for the exported configuration object.
+interface CodeExample { + [key: string]: string; +} + -const react = { +const react: CodeExample = { "/App.jsx": App, }; -export default { +export default: CodeExample { ...react, };apps/docs/content/components/modal/draggable-overflow.ts (1)
1-7: Consider adding TypeScript types for better type safety.The component could benefit from explicit type annotations, especially for the ref and hook returns.
-const App = `import {Modal, ModalContent, ModalHeader, ModalBody, ModalFooter, Button, useDisclosure, useDraggable} from "@nextui-org/react"; +const App = `import {Modal, ModalContent, ModalHeader, ModalBody, ModalFooter, Button, useDisclosure, useDraggable, UseDisclosureReturn} from "@nextui-org/react"; -export default function App() { +export default function App(): JSX.Element { - const {isOpen, onOpen, onOpenChange} = useDisclosure(); + const {isOpen, onOpen, onOpenChange}: UseDisclosureReturn = useDisclosure(); - const targetRef = React.useRef(null); + const targetRef = React.useRef<HTMLDivElement>(null);packages/components/modal/__tests__/modal.test.tsx (3)
11-25: Add TypeScript prop types for better type safety.The
ModalDraggablecomponent should have its props properly typed using TypeScript interface or type definition.+interface ModalDraggableProps { + canOverflow?: boolean; + isDisabled?: boolean; +} -const ModalDraggable = ({canOverflow = false, isDisabled = false}) => { +const ModalDraggable: React.FC<ModalDraggableProps> = ({canOverflow = false, isDisabled = false}) => {
131-132: Refactor viewport mock setup to avoid duplication.The viewport dimension mocks are duplicated across multiple tests. Consider extracting this into a test helper function.
const mockViewport = (width: number, height: number) => { jest.spyOn(document.documentElement, "clientWidth", "get").mockImplementation(() => width); jest.spyOn(document.documentElement, "clientHeight", "get").mockImplementation(() => height); }; // Usage in tests beforeEach(() => { mockViewport(1920, 1080); }); afterEach(() => { jest.restoreAllMocks(); });Also applies to: 152-153, 171-172, 186-187, 201-202
177-179: Extract test constants for better maintainability.The touch event coordinates are hardcoded across multiple tests. Consider extracting these into named constants to make the test intentions clearer.
const DRAG_START_POINT = { pageX: 0, pageY: 0 }; const DRAG_END_POINT = { pageX: 100, pageY: 50 }; const OVERFLOW_POINT = { pageX: 2000, pageY: 1500 }; // Usage in tests fireEvent.touchStart(modalHeader, { changedTouches: [DRAG_START_POINT] }); fireEvent.touchMove(modalHeader, { changedTouches: [DRAG_END_POINT] }); fireEvent.touchEnd(modalHeader, { changedTouches: [DRAG_END_POINT] });Also applies to: 192-194, 209-211
packages/components/modal/stories/modal.stories.tsx (3)
207-229: Consider accessibility and UX improvements for draggable functionalityWhile the basic draggable implementation works, consider the following improvements:
- Add aria-label or aria-description to indicate draggable functionality
- Add visual indicators (e.g., cursor: move) to show the header is draggable
- Consider adding a tooltip or helper text for first-time users
<ModalHeader {...moveProps}>Modal Title</ModalHeader> +<ModalHeader + {...moveProps} + className="cursor-move" + aria-label="Draggable modal header" + title="Drag to move" +> + Modal Title +</ModalHeader>
230-260: Enhance user experience with better controls and persistenceThe implementation could benefit from the following improvements:
- Add tooltips to explain what "Overflow viewport" means
- Consider persisting user preferences in localStorage
- Use more descriptive labels for the switches
- <Switch isSelected={disableDraggable} onValueChange={setDisableDraggable}> - Disable Draggable - </Switch> - <Switch isSelected={canOverflow} onValueChange={setCanOverflow}> - Overflow viewport - </Switch> + <Switch + isSelected={disableDraggable} + onValueChange={setDisableDraggable} + title="Disable dragging functionality" + > + Lock Modal Position + </Switch> + <Switch + isSelected={canOverflow} + onValueChange={setCanOverflow} + title="Allow modal to be dragged outside the viewport" + > + Allow Viewport Overflow + </Switch>
336-350: Document draggable-specific props and add more examplesConsider enhancing the stories with:
- Documentation for draggable-specific props (isDisabled, canOverflow)
- Examples showing different initial positions
- Examples demonstrating drag constraints
Add more story variations like:
export const DraggableWithInitialPosition = { render: DraggableTemplate, args: { ...defaultProps, style: { top: '20%', left: '20%' } }, };apps/docs/content/docs/components/modal.mdx (2)
149-149: Fix grammar in the draggable overflow description.The current text is unclear. Consider revising to:
"Setting overflow to true allows the modal to be dragged beyond the viewport boundaries."🧰 Tools
🪛 LanguageTool
[uncategorized] ~149-~149: Possible missing preposition found.
Context: ... Set overflow to true can drag overflow the viewport. <CodeDemo title="Draggable O...(AI_HYDRA_LEO_MISSING_TO)
141-152: Consider enhancing the draggable feature documentation.While the sections introduce the draggable functionality well, consider adding:
- A note about accessibility implications of draggable modals
- Examples of common use cases
- Explanation of any limitations or browser compatibility considerations
🧰 Tools
🪛 LanguageTool
[uncategorized] ~149-~149: Possible missing preposition found.
Context: ... Set overflow to true can drag overflow the viewport. <CodeDemo title="Draggable O...(AI_HYDRA_LEO_MISSING_TO)
packages/hooks/use-draggable/src/index.ts (4)
12-13: Clarify the description ofisDisabledpropThe current description is grammatically incorrect and might be confusing.
Consider rephrasing it for clarity:
- * Whether to disable the target is draggable. + * Whether the draggable functionality is disabled.
17-18: Improve the description ofcanOverflowpropThe description could be clearer about the effect of this prop.
Consider rewording it to specify its impact on the draggable element:
- * Whether the target can overflow the viewport. + * Whether the draggable element can move outside the viewport boundaries.
95-95: Update the comment to reflect current statusThe comment mentions that the process will become unnecessary once the modal is centered properly.
If the modal centering issue has been resolved or there's a plan to address it, consider updating the comment to reflect the current state or add a TODO for future reference.
110-110: Avoid creating inline styles withinmovePropsInline style objects can cause unnecessary re-renders due to new object references on each render.
Memoize the style object to optimize performance:
+ const cursorStyle = useMemo(() => ({cursor: !isDisabled ? "move" : undefined}), [isDisabled]); return { moveProps: { ...moveProps, - style: {cursor: !isDisabled ? "move" : undefined}, + style: cursorStyle, }, };This ensures the style object reference remains stable unless
isDisabledchanges.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/soft-apricots-sleep.md(1 hunks)apps/docs/content/components/modal/draggable-overflow.ts(1 hunks)apps/docs/content/components/modal/draggable.ts(1 hunks)apps/docs/content/components/modal/index.ts(2 hunks)apps/docs/content/docs/components/modal.mdx(3 hunks)packages/components/modal/__tests__/modal.test.tsx(2 hunks)packages/components/modal/package.json(2 hunks)packages/components/modal/src/index.ts(1 hunks)packages/components/modal/src/modal-header.tsx(1 hunks)packages/components/modal/stories/modal.stories.tsx(4 hunks)packages/hooks/use-draggable/README.md(1 hunks)packages/hooks/use-draggable/package.json(1 hunks)packages/hooks/use-draggable/src/index.ts(1 hunks)packages/hooks/use-draggable/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/hooks/use-draggable/tsconfig.json
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/modal.mdx
[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...ModalFooter } from "@nextui-org/react";, individual: import { Modal...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~149-~149: Possible missing preposition found.
Context: ... Set overflow to true can drag overflow the viewport. <CodeDemo title="Draggable O...
(AI_HYDRA_LEO_MISSING_TO)
packages/hooks/use-draggable/README.md
[style] ~3-~3: Consider using the synonym “brief” (= concise, using a few words, not lasting long) to strengthen your wording.
Context: # @nextui-org/use-draggable A Quick description of the component > This is...
(QUICK_BRIEF)
[typographical] ~17-~17: Usually, a comma is necessary before ‘Yes’ at the end of a sentence. Did you mean “, please”?
Context: .../use-draggable ``` ## Contribution Yes please! See the [contributing guidelines](http...
(COMMA_BEFORE_PLEASE)
🔇 Additional comments (17)
apps/docs/content/components/modal/index.ts (2)
25-26: LGTM! Changes maintain consistency with existing patterns
The new draggable modal features are properly integrated into the modalContent object, following the established pattern of the codebase.
11-12: Verify the existence of imported files
Let's ensure the new import files exist and are properly implemented.
✅ Verification successful
Both imported files exist in the expected location
The verification confirms that both draggable.ts and draggable-overflow.ts files exist in the same directory as the importing file (apps/docs/content/components/modal/), making the imports valid and correctly structured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of new modal example files
# Expected: Both files should exist in the same directory
fd -t f "draggable.ts" -x echo "Found: {}"
fd -t f "draggable-overflow.ts" -x echo "Found: {}"
Length of output: 217
packages/components/modal/src/index.ts (1)
18-18: Verify the dependency declaration.
The new export introduces a dependency on @nextui-org/use-draggable. Let's ensure it's properly declared in the package.json.
✅ Verification successful
Dependency declaration is correct and follows workspace conventions
The dependency is properly declared in the modal's package.json as a workspace dependency ("workspace:*"), and the package exists in the monorepo at packages/hooks/use-draggable. The package is set up correctly with proper configuration and peer dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if @nextui-org/use-draggable is declared in package.json
# Expected: The dependency should be listed in package.json
# Look for package.json in the modal component directory
fd --type f "package.json" packages/components/modal/ --exec cat {} | jq '.dependencies["@nextui-org/use-draggable"]'
Length of output: 133
Script:
#!/bin/bash
# Let's verify if @nextui-org/use-draggable exists in the workspace
fd --type f "package.json" packages/hooks/use-draggable/ --exec cat {}
# Also check if it's published to npm
curl -s https://registry.npmjs.org/@nextui-org/use-draggable | jq '.versions | keys'
Length of output: 1462
packages/components/modal/src/modal-header.tsx (2)
3-3: LGTM: Import statement is correctly placed and necessary
The addition of ReactRef import is required for typing the new ref prop and follows the project's import conventions.
Line range hint 15-44: Verify ref handling in draggable implementation
The component implementation correctly forwards and handles the ref. Let's verify its integration with the draggable functionality.
✅ Verification successful
Ref handling is correctly implemented for draggable functionality
The implementation properly handles refs for draggable functionality:
- The
ModalHeadercomponent correctly forwards its ref throughuseDOMRef - The draggable functionality is implemented through the
useDraggablehook from@nextui-org/use-draggable - The ref is properly passed to the draggable target through
movePropsin both test and story implementations - The modal content correctly manages the ref chain from parent to child components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of ModalHeader ref in draggable implementation
# Search for usages of ModalHeader with ref prop
ast-grep --pattern 'ModalHeader ref={$_}'
# Search for draggable-related files that might use the ModalHeader
rg -l "ModalHeader.*draggable"
Length of output: 76
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Search for any draggable-related code in modal components
rg -l "draggable" packages/components/modal/
# Search for ref usage patterns in modal components
ast-grep --pattern 'useRef<$_>($$$)'
# Search for ModalHeader usage patterns
rg -A 5 "<ModalHeader" packages/components/modal/
# Look for any drag-related hooks or utilities
rg -l "useDrag|DragContext" packages/components/modal/
Length of output: 13118
Script:
#!/bin/bash
# Let's examine the draggable implementation and moveProps usage
# Check the moveProps implementation
rg -A 10 "moveProps" packages/components/modal/
# Look for useDrag hook implementation
rg -A 10 "useDrag" packages/components/modal/
# Check modal content implementation for drag handling
cat packages/components/modal/src/modal-content.tsx
# Look for any drag-related props or types
rg -A 5 "interface.*Props" packages/components/modal/src/
Length of output: 16665
packages/hooks/use-draggable/package.json (2)
2-4: Review version numbering strategy
Starting a new package at version 2.0.0 is unusual. Consider starting at 1.0.0 unless this is intentionally aligned with the main NextUI version or represents a complete rewrite of an existing package.
#!/bin/bash
# Check if this is aligned with other NextUI package versions
fd -e json -x cat {} \; -p "packages/**/package.json" | jq -r '. | select(.name | startswith("@nextui-org")) | {name: .name, version: .version}'52-54: Verify @react-aria/interactions usage
The package depends on @react-aria/interactions for drag functionality. This is a good choice as it provides accessibility features out of the box. However, we should verify that we're using the minimum required features to keep the bundle size optimal.
✅ Verification successful
@react-aria/interactions usage is optimized and appropriate
The package is using only the necessary features from @react-aria/interactions:
- Imports only
useMovehook and related types (MoveMoveEvent,MoveResult) - The implementation specifically uses these for drag functionality
- No unused imports or features are present
The usage is minimal and focused on the core drag-and-drop functionality, which aligns well with the package's purpose. The bundle size impact is already optimized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how @react-aria/interactions is being used
rg -A 5 "from '@react-aria/interactions'"
Length of output: 149
Script:
#!/bin/bash
# Search for imports from @react-aria/interactions with a broader pattern
rg "@react-aria/interactions"
# Also check for any files that might be using this package
fd . packages/hooks/use-draggable -t f -x cat {} \;
Length of output: 14108
apps/docs/content/components/modal/draggable.ts (2)
1-45: Add test coverage for draggable functionality
While the implementation meets the requirements from issue #2647, consider adding the following test scenarios:
- Verify dragging behavior with mouse and touch events
- Test modal positioning after drag
- Ensure keyboard accessibility is maintained
- Test behavior when dragging near viewport edges
#!/bin/bash
# Check for existing modal tests
rg -l "test|spec" --glob "**/{modal,draggable}*"11-34: 🛠️ Refactor suggestion
Enhance accessibility and user experience
The modal implementation needs improvements in accessibility and user experience:
- Missing aria-label for screen readers
- No visual indication that the modal is draggable
- No bounds checking when dragging
Apply these changes:
- <Modal ref={targetRef} isOpen={isOpen} onOpenChange={onOpenChange}>
+ <Modal
+ ref={targetRef}
+ isOpen={isOpen}
+ onOpenChange={onOpenChange}
+ aria-label="Draggable modal"
+ >
<ModalContent>
{(onClose) => (
<>
- <ModalHeader {...moveProps} className="flex flex-col gap-1">Modal Title</ModalHeader>
+ <ModalHeader
+ {...moveProps}
+ className="flex flex-col gap-1 cursor-move select-none"
+ aria-label="Drag to move modal"
+ >
+ <div className="flex items-center gap-2">
+ <span aria-hidden="true">⋮</span>
+ Modal Title
+ </div>
+ </ModalHeader>Additionally, consider adding bounds checking to prevent dragging outside viewport:
apps/docs/content/components/modal/draggable-overflow.ts (2)
39-45: LGTM! Export configuration is properly structured.
The export format is correct for documentation examples.
1-45: Verify draggable modal implementation coverage.
Let's ensure the draggable functionality is properly tested and consistently implemented.
✅ Verification successful
Let me try a different approach to gather the necessary information.
Let me check the test coverage and implementation details of the draggable functionality.
Draggable modal implementation is well-tested and properly implemented
The verification shows that:
- The
useDraggablehook is thoroughly tested with various scenarios including overflow, disabled state, and mobile interactions - Modal component has dedicated test cases for draggable functionality
- The implementation includes proper examples in stories and documentation
- The feature is properly bounded to viewport limits when
canOverflowis false - Touch events are properly handled for mobile devices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage and other modal implementations
# Test 1: Check for draggable modal tests
echo "Checking for draggable modal tests..."
rg -l "useDraggable.*modal" -t test
# Test 2: Find other modal implementations that might benefit from draggable functionality
echo "Finding other modal implementations..."
ast-grep --pattern 'const $_ = () => {
$$$
return (
$$$
<Modal
$$$
</Modal>
$$$
)
}'
Length of output: 353
Script:
#!/bin/bash
# Let's check for test files and modal implementations more thoroughly
# Check for test files related to useDraggable
echo "Checking for draggable tests..."
rg -l "useDraggable"
# Find Modal component usage patterns
echo -e "\nChecking Modal implementations..."
rg -l "<Modal"
# Look for useDraggable hook implementation
echo -e "\nChecking useDraggable implementation..."
rg -l "useDraggable" --type ts --type tsx
# Check for draggable-related test files
echo -e "\nChecking specific test files..."
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts
Length of output: 4714
Script:
#!/bin/bash
# Check the modal test file and useDraggable implementation
# Check modal test content
echo "Checking modal test content..."
cat packages/components/modal/__tests__/modal.test.tsx
# Check useDraggable implementation
echo -e "\nChecking useDraggable implementation..."
cat packages/hooks/use-draggable/src/index.ts
# Check modal stories for draggable examples
echo -e "\nChecking modal stories..."
cat packages/components/modal/stories/modal.stories.tsx
Length of output: 19978
packages/components/modal/package.json (2)
45-45: LGTM: Addition of use-draggable dependency
The addition of @nextui-org/use-draggable as a runtime dependency is appropriate for implementing the draggable modal feature.
67-67: LGTM: Addition of switch component for development
The addition of @nextui-org/switch as a development dependency is appropriate as it's used for toggling draggable behavior in the component stories.
packages/components/modal/stories/modal.stories.tsx (1)
9-9: LGTM: Required imports added correctly
The new imports for Switch component and useDraggable hook are properly added and necessary for implementing the draggable modal functionality.
Also applies to: 21-21
apps/docs/content/docs/components/modal.mdx (2)
45-56: LGTM! Import statements are well-formatted.
The import statements are properly organized with consistent spacing and line breaks.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...ModalFooter } from "@nextui-org/react";, individual: import { Modal...
(UNLIKELY_OPENING_PUNCTUATION)
77-77: LGTM! Clear explanation of modal's default behavior.
The documentation clearly explains the modal's dismissal behavior and keyboard interaction.
packages/hooks/use-draggable/src/index.ts (1)
90-92: 🛠️ Refactor suggestion
Include preventDefault in the useEffect dependencies
The preventDefault callback should be included in the dependency array to ensure consistent behavior.
Add preventDefault to the dependencies:
- }, [isDisabled]);
+ }, [isDisabled, preventDefault]);This guarantees that the effect updates when preventDefault changes.
Likely invalid or redundant comment.
| --- | ||
| "@nextui-org/modal": patch | ||
| "@nextui-org/use-draggable": patch | ||
| --- | ||
|
|
||
| Add draggable modal (#2647) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using minor version bumps instead of patches.
According to semantic versioning guidelines, adding new functionality (like the draggable modal feature) typically warrants a minor version bump rather than a patch. Patch versions are reserved for backward-compatible bug fixes.
Apply this diff to update the version types:
---
-"@nextui-org/modal": patch
-"@nextui-org/use-draggable": patch
+"@nextui-org/modal": minor
+"@nextui-org/use-draggable": minor
---📝 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.
| --- | |
| "@nextui-org/modal": patch | |
| "@nextui-org/use-draggable": patch | |
| --- | |
| Add draggable modal (#2647) | |
| --- | |
| "@nextui-org/modal": minor | |
| "@nextui-org/use-draggable": minor | |
| --- | |
| Add draggable modal (#2647) |
| "devDependencies": { | ||
| "clean-package": "2.2.0", | ||
| "react": "^18.0.0" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential development dependencies
The devDependencies section is missing some important packages:
@types/reactfor TypeScript definitions- Testing framework (jest/vitest)
- ESLint dependencies for code quality
Apply this diff to add essential dev dependencies:
"devDependencies": {
"clean-package": "2.2.0",
- "react": "^18.0.0"
+ "react": "^18.0.0",
+ "@types/react": "^18.0.0",
+ "vitest": "^0.34.0",
+ "@testing-library/react": "^14.0.0",
+ "@testing-library/react-hooks": "^8.0.1"
}Committable suggestion skipped: line range outside the PR's diff.
| "scripts": { | ||
| "build": "tsup src --dts", | ||
| "build:fast": "tsup src", | ||
| "dev": "pnpm build:fast --watch", | ||
| "clean": "rimraf dist .turbo", | ||
| "typecheck": "tsc --noEmit", | ||
| "prepack": "clean-package", | ||
| "postpack": "clean-package restore" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test script to package scripts
The scripts section should include a test script to ensure the hook's functionality is properly tested.
Apply this diff to add the test script:
"scripts": {
"build": "tsup src --dts",
"build:fast": "tsup src",
"dev": "pnpm build:fast --watch",
"clean": "rimraf dist .turbo",
"typecheck": "tsc --noEmit",
"prepack": "clean-package",
- "postpack": "clean-package restore"
+ "postpack": "clean-package restore",
+ "test": "vitest run",
+ "test:watch": "vitest"
}Committable suggestion skipped: line range outside the PR's diff.
| const App = `import {Modal, ModalContent, ModalHeader, ModalBody, ModalFooter, Button, useDisclosure, useDraggable} from "@nextui-org/react"; | ||
|
|
||
| export default function App() { | ||
| const {isOpen, onOpen, onOpenChange} = useDisclosure(); | ||
| const targetRef = React.useRef(null); | ||
| const {moveProps} = useDraggable({ targetRef }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing React import and improve type safety
The code is missing the React import despite using React.useRef. Additionally, the ref type should be explicitly defined for better type safety.
Apply these changes:
-const App = `import {Modal, ModalContent, ModalHeader, ModalBody, ModalFooter, Button, useDisclosure, useDraggable} from "@nextui-org/react";
+const App = `import React from "react";
+import {Modal, ModalContent, ModalHeader, ModalBody, ModalFooter, Button, useDisclosure, useDraggable} from "@nextui-org/react";
export default function App() {
const {isOpen, onOpen, onOpenChange} = useDisclosure();
- const targetRef = React.useRef(null);
+ const targetRef = React.useRef<HTMLDivElement>(null);
const {moveProps} = useDraggable({ targetRef });📝 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 App = `import {Modal, ModalContent, ModalHeader, ModalBody, ModalFooter, Button, useDisclosure, useDraggable} from "@nextui-org/react"; | |
| export default function App() { | |
| const {isOpen, onOpen, onOpenChange} = useDisclosure(); | |
| const targetRef = React.useRef(null); | |
| const {moveProps} = useDraggable({ targetRef }); | |
| const App = `import React from "react"; | |
| import {Modal, ModalContent, ModalHeader, ModalBody, ModalFooter, Button, useDisclosure, useDraggable} from "@nextui-org/react"; | |
| export default function App() { | |
| const {isOpen, onOpen, onOpenChange} = useDisclosure(); | |
| const targetRef = React.useRef<HTMLDivElement>(null); | |
| const {moveProps} = useDraggable({ targetRef }); |
| <Modal ref={targetRef} isOpen={isOpen} onOpenChange={onOpenChange}> | ||
| <ModalContent> | ||
| {(onClose) => ( | ||
| <> | ||
| <ModalHeader {...moveProps} className="flex flex-col gap-1">Modal Title</ModalHeader> | ||
| <ModalBody> | ||
| <p> | ||
| Lorem ipsum dolor sit amet, consectetur adipiscing elit. | ||
| Nullam pulvinar risus non risus hendrerit venenatis. | ||
| Pellentesque sit amet hendrerit risus, sed porttitor quam. | ||
| </p> | ||
| </ModalBody> | ||
| <ModalFooter> | ||
| <Button color="danger" variant="light" onPress={onClose}> | ||
| Close | ||
| </Button> | ||
| <Button color="primary" onPress={onClose}> | ||
| Action | ||
| </Button> | ||
| </ModalFooter> | ||
| </> | ||
| )} | ||
| </ModalContent> | ||
| </Modal> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility with ARIA labels and button types.
The modal and its interactive elements should have proper accessibility attributes.
- <Modal ref={targetRef} isOpen={isOpen} onOpenChange={onOpenChange}>
+ <Modal
+ ref={targetRef}
+ isOpen={isOpen}
+ onOpenChange={onOpenChange}
+ aria-label="Draggable modal example"
+ >
<ModalContent>
{(onClose) => (
<>
- <ModalHeader {...moveProps} className="flex flex-col gap-1">Modal Title</ModalHeader>
+ <ModalHeader {...moveProps} className="flex flex-col gap-1 cursor-move">Modal Title</ModalHeader>
<ModalBody>
<p>
Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Nullam pulvinar risus non risus hendrerit venenatis.
Pellentesque sit amet hendrerit risus, sed porttitor quam.
</p>
</ModalBody>
<ModalFooter>
- <Button color="danger" variant="light" onPress={onClose}>
+ <Button
+ color="danger"
+ variant="light"
+ onPress={onClose}
+ type="button"
+ aria-label="Close modal"
+ >
Close
</Button>
- <Button color="primary" onPress={onClose}>
+ <Button
+ color="primary"
+ onPress={onClose}
+ type="button"
+ aria-label="Confirm action"
+ >
Action
</Button>📝 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.
| <Modal ref={targetRef} isOpen={isOpen} onOpenChange={onOpenChange}> | |
| <ModalContent> | |
| {(onClose) => ( | |
| <> | |
| <ModalHeader {...moveProps} className="flex flex-col gap-1">Modal Title</ModalHeader> | |
| <ModalBody> | |
| <p> | |
| Lorem ipsum dolor sit amet, consectetur adipiscing elit. | |
| Nullam pulvinar risus non risus hendrerit venenatis. | |
| Pellentesque sit amet hendrerit risus, sed porttitor quam. | |
| </p> | |
| </ModalBody> | |
| <ModalFooter> | |
| <Button color="danger" variant="light" onPress={onClose}> | |
| Close | |
| </Button> | |
| <Button color="primary" onPress={onClose}> | |
| Action | |
| </Button> | |
| </ModalFooter> | |
| </> | |
| )} | |
| </ModalContent> | |
| </Modal> | |
| <Modal | |
| ref={targetRef} | |
| isOpen={isOpen} | |
| onOpenChange={onOpenChange} | |
| aria-label="Draggable modal example" | |
| > | |
| <ModalContent> | |
| {(onClose) => ( | |
| <> | |
| <ModalHeader {...moveProps} className="flex flex-col gap-1 cursor-move">Modal Title</ModalHeader> | |
| <ModalBody> | |
| <p> | |
| Lorem ipsum dolor sit amet, consectetur adipiscing elit. | |
| Nullam pulvinar risus non risus hendrerit venenatis. | |
| Pellentesque sit amet hendrerit risus, sed porttitor quam. | |
| </p> | |
| </ModalBody> | |
| <ModalFooter> | |
| <Button | |
| color="danger" | |
| variant="light" | |
| onPress={onClose} | |
| type="button" | |
| aria-label="Close modal" | |
| > | |
| Close | |
| </Button> | |
| <Button | |
| color="primary" | |
| onPress={onClose} | |
| type="button" | |
| aria-label="Confirm action" | |
| > | |
| Action | |
| </Button> | |
| </ModalFooter> | |
| </> | |
| )} | |
| </ModalContent> | |
| </Modal> |
| it("should be rendered a draggable modal", () => { | ||
| // mock viewport size to 1920x1080 | ||
| jest.spyOn(document.documentElement, "clientWidth", "get").mockImplementation(() => 1920); | ||
| jest.spyOn(document.documentElement, "clientHeight", "get").mockImplementation(() => 1080); | ||
|
|
||
| const wrapper = render(<ModalDraggable />); | ||
|
|
||
| const modal = wrapper.getByRole("dialog"); | ||
| const modalHeader = wrapper.getByText("Modal header"); | ||
|
|
||
| fireEvent.touchStart(modalHeader, {changedTouches: [{pageX: 0, pageY: 0}]}); | ||
| fireEvent.touchMove(modalHeader, {changedTouches: [{pageX: 100, pageY: 50}]}); | ||
| fireEvent.touchEnd(modalHeader, {changedTouches: [{pageX: 100, pageY: 50}]}); | ||
|
|
||
| expect(() => wrapper.unmount()).not.toThrow(); | ||
| expect(document.documentElement.clientWidth).toBe(1920); | ||
| expect(document.documentElement.clientHeight).toBe(1080); | ||
| expect(modalHeader.style.cursor).toBe("move"); | ||
| expect(modal.style.transform).toBe("translate(100px, 50px)"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add mouse event tests for desktop interaction.
The draggable modal is currently only tested with touch events. Add equivalent mouse event tests (mouseDown, mouseMove, mouseUp) to ensure desktop drag functionality works correctly.
it("should be draggable with mouse events", () => {
jest.spyOn(document.documentElement, "clientWidth", "get").mockImplementation(() => 1920);
jest.spyOn(document.documentElement, "clientHeight", "get").mockImplementation(() => 1080);
const wrapper = render(<ModalDraggable />);
const modalHeader = wrapper.getByText("Modal header");
fireEvent.mouseDown(modalHeader, { clientX: 0, clientY: 0 });
fireEvent.mouseMove(modalHeader, { clientX: 100, clientY: 50 });
fireEvent.mouseUp(modalHeader);
expect(wrapper.getByRole("dialog").style.transform).toBe("translate(100px, 50px)");
});| targetRef.current.style.transform = `translate(${moveX}px, ${moveY}px)`; | ||
| } | ||
| }, | ||
| [isDisabled, transform, boundary.current, canOverflow, targetRef?.current], |
There was a problem hiding this comment.
Adjust the dependency array in onMove callback
Mutable objects like transform.current, boundary.current, and targetRef?.current shouldn't be included in the dependency array.
Modify the dependencies to include only the necessary variables:
- [isDisabled, transform, boundary.current, canOverflow, targetRef?.current]
+ [isDisabled, canOverflow, targetRef]This prevents potential bugs due to stale closures and unnecessary re-renders.
📝 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.
| [isDisabled, transform, boundary.current, canOverflow, targetRef?.current], | |
| [isDisabled, canOverflow, targetRef], |
| export function useDraggable(props: UseDraggableProps): MoveResult { | ||
| const {targetRef, isDisabled = false, canOverflow = false} = props; | ||
| const boundary = useRef({minLeft: 0, minTop: 0, maxLeft: 0, maxTop: 0}); | ||
| let transform = {offsetX: 0, offsetY: 0}; |
There was a problem hiding this comment.
Use useRef for transform to prevent stale closures
The transform object is being reassigned within callbacks, which can lead to issues due to stale closures since it's not preserved between renders.
Consider using useRef to maintain a consistent reference:
- let transform = {offsetX: 0, offsetY: 0};
+ const transform = useRef({offsetX: 0, offsetY: 0});Update references of transform to transform.current:
- const {offsetX, offsetY} = transform;
+ const {offsetX, offsetY} = transform.current;When updating transform:
- transform = {
+ transform.current = {Ensure that all uses of transform are updated accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| maxLeft, | ||
| maxTop, | ||
| }; | ||
| }, [transform, targetRef?.current]); |
There was a problem hiding this comment.
Correct the dependency array in onMoveStart
Including transform and targetRef?.current in the dependency array may cause unnecessary re-renders or misses updates due to their mutable nature.
After switching transform to use useRef, adjust the dependencies:
- }, [transform, targetRef?.current]);
+ }, [targetRef]);This ensures that the callback updates properly when relevant references change.
Committable suggestion skipped: line range outside the PR's diff.
Closes #2647
Original PR: #2818
📝 Description
This pr adds dragging behavior to
modalby providinguseDraggablehook.⛳️ Current behavior (updates)
No
🚀 New behavior
This pr adds dragging behavior to
modalby providinguseDraggablehook.💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
No
👀Preview
Screen-2024-04-20-193626.mp4
Summary by CodeRabbit
New Features
Documentation
@nextui-org/use-draggablepackage.Bug Fixes
Tests