feature: allow modal to be resized in width #2838
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
📝 Walkthrough""" WalkthroughThe Modal component was updated to allow users to dynamically resize its width using a new resize handle. The width prop was renamed to initialWidth, internal state was added to track resizing, and mouse event handlers were implemented. Corresponding CSS was introduced for the resize handle and to disable transitions during resizing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModalComponent
participant Window
User->>ModalComponent: MouseDown on resize handle
ModalComponent->>Window: Attach MouseMove and MouseUp listeners
User->>Window: MouseMove (dragging)
Window->>ModalComponent: handleMouseMove (update width)
User->>Window: MouseUp
Window->>ModalComponent: handleMouseUp (end resizing)
ModalComponent->>Window: Remove MouseMove and MouseUp listeners
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Suggested reviewers
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 code ERR_SSL_WRONG_VERSION_NUMBER 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 (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/components/Modal/Modal.scss (1)
103-112: Consider enhancing the resize handle's visual feedback.The resize handle might not be immediately discoverable to users. Consider adding hover states and a more visible indicator.
.resizeHandle { width: 16px; height: 16px; position: absolute; bottom: 0; right: 0; cursor: se-resize; - background: rgba(255, 255, 255, 0.5); + background: linear-gradient(135deg, transparent 50%, rgba(0, 0, 0, 0.2) 50%); z-index: 10; + transition: background 0.2s ease; + + &:hover { + background: linear-gradient(135deg, transparent 40%, rgba(0, 0, 0, 0.3) 40%); + } }src/components/Modal/Modal.react.js (1)
62-72: Optimize event listener attachment for better performance.The mouse move event listener is always active even when not resizing, which could impact performance.
Only attach the mousemove listener when resizing starts:
const handleMouseDown = (e) => { e.preventDefault(); resizing.current = true; if (modalRef.current) { modalRef.current.classList.add(styles.noTransition); } + document.addEventListener('mousemove', handleMouseMove); + document.addEventListener('mouseup', handleMouseUp); }; const handleMouseUp = () => { resizing.current = false; if (modalRef.current) { modalRef.current.classList.remove(styles.noTransition); } + document.removeEventListener('mousemove', handleMouseMove); + document.removeEventListener('mouseup', handleMouseUp); }; -useEffect(() => { - document.addEventListener('mousemove', handleMouseMove); - document.addEventListener('mouseup', handleMouseUp); - - return () => { - document.removeEventListener('mousemove', handleMouseMove); - document.removeEventListener('mouseup', handleMouseUp); - }; -}, []); +useEffect(() => { + return () => { + // Cleanup on unmount + document.removeEventListener('mousemove', handleMouseMove); + document.removeEventListener('mouseup', handleMouseUp); + }; +}, [handleMouseMove, handleMouseUp]);🧰 Tools
🪛 GitHub Check: Lint
[failure] 65-65:
Expected indentation of 4 spaces but found 2
[failure] 64-64:
Expected indentation of 6 spaces but found 4
[failure] 63-63:
Expected indentation of 4 spaces but found 2
[failure] 62-62:
Expected indentation of 2 spaces but found 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Modal/Modal.react.js(4 hunks)src/components/Modal/Modal.scss(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
src/components/Modal/Modal.react.js
[failure] 65-65:
Expected indentation of 4 spaces but found 2
[failure] 64-64:
Expected indentation of 6 spaces but found 4
[failure] 63-63:
Expected indentation of 4 spaces but found 2
[failure] 62-62:
Expected indentation of 2 spaces but found 0
[failure] 60-60:
Expected indentation of 2 spaces but found 0
[failure] 59-59:
Expected indentation of 4 spaces but found 2
[failure] 58-58:
Expected indentation of 6 spaces but found 4
[failure] 57-57:
Expected indentation of 4 spaces but found 2
[failure] 56-56:
Expected indentation of 4 spaces but found 2
[failure] 55-55:
Expected indentation of 4 spaces but found 2
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
Uffizzi Ephemeral Environment
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/components/Modal/Modal.react.js (4)
52-58: Indentation issues persist in handleMouseDown function.The indentation in this function is still inconsistent with the rest of the file, as previously identified.
60-73: Memory leak addressed but indentation issues remain.Good improvement using useCallback with proper dependencies to prevent memory leaks. However, the indentation issues within the function persist as previously noted.
75-80: Memory leak addressed but indentation issues remain.Good improvement using useCallback. However, the indentation issues within the function persist as previously noted.
152-152: 🛠️ Refactor suggestionAccessibility and usability improvements needed for resize handle.
The resize handle lacks several important features:
- Keyboard accessibility (as previously noted)
- ARIA labels for screen readers
- Touch device considerations (handle might be too small)
- Visual feedback for resize capability
- <div className={styles.resizeHandle} onMouseDown={handleMouseDown} /> + <div + className={styles.resizeHandle} + onMouseDown={handleMouseDown} + role="button" + tabIndex={0} + aria-label="Resize modal width" + title="Drag to resize modal width" + />
🧹 Nitpick comments (2)
src/components/Modal/Modal.react.js (2)
129-133: Consider performance optimization for frequent style updates.The dynamic width updates on every mouse move could impact performance. Consider throttling or using CSS transforms instead of direct style updates.
- style={{ width: currentWidth }} + style={{ width: currentWidth, transform: 'translateZ(0)' }}The translateZ(0) creates a composite layer that can improve rendering performance during frequent updates.
52-90: Consider adding props to control resize behavior.The resize functionality is always enabled and lacks configuration options. Consider adding props like
resizable,minWidth,maxWidthfor better control.customFooter, textModal = false, width, initialWidth = width ?? 500, + resizable = true, + minWidth = 300, + maxWidth, continueText,Then update the clamp logic:
- const clampedWidth = Math.min(window.innerWidth, Math.max(initialWidth, newWidth)); + const maxAllowed = maxWidth || window.innerWidth; + const clampedWidth = Math.min(maxAllowed, Math.max(minWidth, newWidth));And conditionally render the resize handle:
{wrappedChildren} {footer} - <div className={styles.resizeHandle} onMouseDown={handleMouseDown} /> + {resizable && <div className={styles.resizeHandle} onMouseDown={handleMouseDown} />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Modal/Modal.react.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (5)
src/components/Modal/Modal.react.js (5)
13-13: LGTM: Appropriate React hooks imported for resize functionality.The addition of useState, useEffect, useRef, and useCallback imports is correct and necessary for implementing the modal resizing feature.
42-42: Good backward compatibility solution.The implementation
initialWidth = width ?? 500effectively addresses the breaking change concern by providing backward compatibility while transitioning to the new prop name.
48-50: LGTM: Proper state and ref setup for resize functionality.The refs and state initialization are correctly implemented for tracking modal reference, current width, and resize state.
82-90: Excellent improvement: Memory leak issue resolved.The useEffect now properly includes handleMouseMove and handleMouseUp in the dependency array, preventing stale closures and memory leaks.
186-187: PropTypes documentation correctly updated.Good addition of both
widthandinitialWidthprop documentation, clearly indicating the legacy status of thewidthprop.
Uffizzi Ephemeral Environment
|
|
Please remove the resizing functionality from the modal corner and add it to the existing text input field corner. And make it resizable horizontally and verticall at the same time. 
The issue with the resizing handle in the modal corner is that it's close to the "Save" button, which means one could accidentally save an unfinished entry when trying to resize. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/Modal/Modal.react.js (1)
41-42: Breaking change already flagged in previous reviews.The renaming of
widthtoinitialWidthcreates a breaking change that affects existing Modal component usage.
🧹 Nitpick comments (2)
src/components/Modal/Modal.react.js (2)
48-50: Improve the resizing state structure.The resizing ref object structure could be more maintainable and the variable naming could be clearer.
- const resizing = useRef({ active: false, side: null, startX: 0, startWidth: 0 }); + const resizing = useRef({ + active: false, + side: null, + startX: 0, + startWidth: 0 + });
207-208: Update prop documentation to reflect current implementation.The prop descriptions should clarify the current behavior and the relationship between the legacy and new props.
- width: PropTypes.number.describe('custom width of modal (legacy prop, prefer initialWidth).'), - initialWidth: PropTypes.number.describe('custom width of modal.'), + width: PropTypes.number.describe('Legacy prop for modal width. Use initialWidth instead.'), + initialWidth: PropTypes.number.describe('Initial width of the resizable modal. Defaults to 500px.'),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Modal/Modal.react.js(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
src/components/Modal/Modal.react.js
[failure] 69-69:
Expected { after 'if' condition
[failure] 57-57:
Expected { after 'if' condition
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (2)
src/components/Modal/Modal.react.js (2)
13-13: LGTM: React hooks import updated appropriately.The addition of the required React hooks for the resize functionality is correct.
91-98: Event listener management looks good.The useEffect properly handles adding and removing global event listeners with correct dependencies.
| const handleMouseMove = useCallback( | ||
| (e) => { | ||
| if (!resizing.current.active || !modalRef.current) return; | ||
| const { side, startX, startWidth } = resizing.current; | ||
| if (side === 'right') { | ||
| let newWidth = startWidth + 2 * (e.clientX - startX); | ||
| newWidth = Math.max(minWidth, newWidth); | ||
| setCurrentWidth(newWidth); | ||
| } else if (side === 'left') { | ||
| let newWidth = startWidth - 2 * (e.clientX - startX); | ||
| newWidth = Math.max(minWidth, newWidth); | ||
| setCurrentWidth(newWidth); | ||
| } | ||
| }, | ||
| [minWidth] | ||
| ); |
There was a problem hiding this comment.
Fix missing braces and problematic resize logic.
Multiple issues with this implementation:
- Missing braces after if condition (line 69)
- Arbitrary 2x multiplier makes resizing overly sensitive
- No maximum width constraint
- if (!resizing.current.active || !modalRef.current) return;
+ if (!resizing.current.active || !modalRef.current) {
+ return;
+ }- let newWidth = startWidth + 2 * (e.clientX - startX);
+ let newWidth = startWidth + (e.clientX - startX);- newWidth = Math.max(minWidth, newWidth);
+ newWidth = Math.min(window.innerWidth - 40, Math.max(minWidth, newWidth));📝 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 handleMouseMove = useCallback( | |
| (e) => { | |
| if (!resizing.current.active || !modalRef.current) return; | |
| const { side, startX, startWidth } = resizing.current; | |
| if (side === 'right') { | |
| let newWidth = startWidth + 2 * (e.clientX - startX); | |
| newWidth = Math.max(minWidth, newWidth); | |
| setCurrentWidth(newWidth); | |
| } else if (side === 'left') { | |
| let newWidth = startWidth - 2 * (e.clientX - startX); | |
| newWidth = Math.max(minWidth, newWidth); | |
| setCurrentWidth(newWidth); | |
| } | |
| }, | |
| [minWidth] | |
| ); | |
| const handleMouseMove = useCallback( | |
| (e) => { | |
| if (!resizing.current.active || !modalRef.current) { | |
| return; | |
| } | |
| const { side, startX, startWidth } = resizing.current; | |
| if (side === 'right') { | |
| let newWidth = startWidth + (e.clientX - startX); | |
| newWidth = Math.min(window.innerWidth - 40, Math.max(minWidth, newWidth)); | |
| setCurrentWidth(newWidth); | |
| } else if (side === 'left') { | |
| let newWidth = startWidth - 2 * (e.clientX - startX); | |
| newWidth = Math.max(minWidth, newWidth); | |
| setCurrentWidth(newWidth); | |
| } | |
| }, | |
| [minWidth] | |
| ); |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 69-69:
Expected { after 'if' condition
🤖 Prompt for AI Agents
In src/components/Modal/Modal.react.js lines 67 to 82, add braces after the if
condition on line 69 to properly enclose the block. Remove the arbitrary 2x
multiplier in the width calculation to make resizing less sensitive. Also,
introduce a maximum width constraint and ensure the new width is clamped between
minWidth and maxWidth before calling setCurrentWidth.
| {/* Left resize handle */} | ||
| <div | ||
| className={styles.resizeHandleLeft} | ||
| style={{ position: 'absolute', left: 0, top: 0, width: 8, height: '100%', cursor: 'ew-resize', zIndex: 10 }} | ||
| onMouseDown={handleMouseDown('left')} | ||
| /> | ||
| {/* Right resize handle */} | ||
| <div | ||
| className={styles.resizeHandleRight} | ||
| style={{ position: 'absolute', right: 0, top: 0, width: 8, height: '100%', cursor: 'ew-resize', zIndex: 10 }} | ||
| onMouseDown={handleMouseDown('right')} | ||
| /> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Implementation doesn't match reviewer requirements.
Critical mismatch with reviewer feedback:
- Reviewer specifically requested moving resize handle to text input field corner, not modal edges
- Reviewer requested both horizontal AND vertical resizing, but this only implements horizontal
- Handles are positioned at modal edges which conflicts with the Save button as mentioned in PR comments
The current implementation needs to be redesigned to:
- Place resize handle on text input field corner (not modal edges)
- Support both horizontal and vertical resizing
- Avoid interference with the Save button
Should I help redesign this to match the reviewer's specifications?
Redesign resize handles to meet spec
The current code in src/components/Modal/Modal.react.js (lines 144–155) only adds horizontal resize grips at the modal edges and will overlap the Save button. The requirements call for a corner-based handle on the text input field that supports both horizontal and vertical resizing.
Please update as follows:
- Move the resize handle into the text-input wrapper (not on the modal container).
- Position it at the bottom-right corner of that input (e.g. wrap
<input>in aposition: relativecontainer, then absolutely position the handle atbottom: 0; right: 0). - Replace the two edge handles with a single corner handle that uses a diagonal cursor (
nwse-resize) and invokes your bidirectional resize logic. - Implement vertical resizing (e.g.
handleMouseDown('bottom')or a unifiedhandleMouseDown('corner')that handles both axes). - Verify it doesn’t collide with the Save button (adjust z-index/offsets if needed).
Example diff snippet:
- {/* Left resize handle */}
- <div
- className={styles.resizeHandleLeft}
- style={{ position: 'absolute', left: 0, top: 0, width: 8, height: '100%', cursor: 'ew-resize', zIndex: 10 }}
- onMouseDown={handleMouseDown('left')}
- />
- {/* Right resize handle */}
- <div
- className={styles.resizeHandleRight}
- style={{ position: 'absolute', right: 0, top: 0, width: 8, height: '100%', cursor: 'ew-resize', zIndex: 10 }}
- onMouseDown={handleMouseDown('right')}
- />
+ {/* Corner resize handle on text input */}
+ <div
+ className={styles.resizeHandleCorner}
+ style={{
+ position: 'absolute',
+ bottom: 0,
+ right: 0,
+ width: 8,
+ height: 8,
+ cursor: 'nwse-resize',
+ zIndex: 10
+ }}
+ onMouseDown={handleMouseDown('corner')}
+ />Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Modal/Modal.react.js around lines 144 to 155, replace the two
horizontal resize handles positioned at the modal edges with a single resize
handle placed at the bottom-right corner of the text input field's wrapper. Wrap
the input in a relative container and absolutely position the handle at bottom:
0 and right: 0 with a diagonal cursor (nwse-resize). Update the onMouseDown
handler to support bidirectional resizing (both horizontal and vertical) by
using a unified event like handleMouseDown('corner'). Ensure the handle's
z-index and positioning prevent overlap with the Save button.
| const handleMouseDown = (side) => (e) => { | ||
| e.preventDefault(); | ||
| if (!modalRef.current) return; | ||
| resizing.current = { | ||
| active: true, | ||
| side, | ||
| startX: e.clientX, | ||
| startWidth: currentWidth, | ||
| }; | ||
| modalRef.current.classList.add(styles.noTransition); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix missing braces and address reviewer feedback on handle placement.
Two critical issues:
- Missing braces after if condition (static analysis error)
- Current implementation doesn't match reviewer's request to move resize handle to text input corner
- if (!modalRef.current) return;
+ if (!modalRef.current) {
+ return;
+ }More importantly, according to PR comments, the reviewer requested moving the resize handle from the modal corner to the text input field corner to avoid conflicts with the Save button. The current implementation still places handles on modal edges.
🏁 Script executed:
#!/bin/bash
# Locate handleMouseDown definition and its usage in the Modal component
echo "---- handleMouseDown definition ----"
rg -n "handleMouseDown" -C3 src/components/Modal/Modal.react.js
echo "---- Context around resizing.current usage ----"
rg -n "resizing.current" -C3 src/components/Modal/Modal.react.js
echo "---- Search for render of resize handles ----"
rg -n "<.*handle" -C3 src/components/Modal/Modal.react.js || echo "No obvious handle elements found"
echo "---- ESLint curly rule configuration ----"
grep -R "\"curly\"" -n .eslintrc* || echo "No curly rule found in ESLint config"Length of output: 2448
Add braces to the early return and relocate resize handles to the input field corners
The if (!modalRef.current) return; remains a single‐line statement and needs braces for consistency (and to satisfy any “curly” lint rules). More importantly, the resize handles are still rendered at the modal edges instead of at the text input’s corners as requested.
• File: src/components/Modal/Modal.react.js
– Lines 55–57: wrap the early return in braces
diff - if (!modalRef.current) return; + if (!modalRef.current) { + return; + }
• File: src/components/Modal/Modal.react.js
– Lines 145–155: the <div className={styles.resizeHandleLeft}> and <div className={styles.resizeHandleRight}> are still absolutely positioned at the modal’s left/right edges. Per PR feedback, these handles should be moved inside the text input wrapper (e.g. attach to top‐left and top‐right of the input element) to avoid overlapping the Save button.
– Adjust the markup and positioning so that the handles render at the input field’s corners rather than the modal frame.
📝 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 handleMouseDown = (side) => (e) => { | |
| e.preventDefault(); | |
| if (!modalRef.current) return; | |
| resizing.current = { | |
| active: true, | |
| side, | |
| startX: e.clientX, | |
| startWidth: currentWidth, | |
| }; | |
| modalRef.current.classList.add(styles.noTransition); | |
| }; | |
| const handleMouseDown = (side) => (e) => { | |
| e.preventDefault(); | |
| if (!modalRef.current) { | |
| return; | |
| } | |
| resizing.current = { | |
| active: true, | |
| side, | |
| startX: e.clientX, | |
| startWidth: currentWidth, | |
| }; | |
| modalRef.current.classList.add(styles.noTransition); | |
| }; |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 57-57:
Expected { after 'if' condition
🤖 Prompt for AI Agents
In src/components/Modal/Modal.react.js around lines 55 to 57, add braces around
the early return statement after the if condition to comply with curly brace
lint rules. Then, in lines 145 to 155, move the resize handle elements from the
modal edges to be inside the text input wrapper, positioning them at the
top-left and top-right corners of the input field instead of the modal frame, to
prevent overlap with the Save button. Adjust the JSX markup and CSS styles
accordingly to reflect this new placement.
New Pull Request Checklist
Issue Description
Closes: #1879
Approach
TODOs before merging
Summary by CodeRabbit
New Features
Style