[PE-255] fix: remove drag handles from content within table cells#6487
[PE-255] fix: remove drag handles from content within table cells#6487sriramveeraghanta merged 4 commits intopreviewfrom
Conversation
WalkthroughThe pull request introduces modifications to the side menu and drag handle positioning logic in the editor. Changes focus on refining how elements are selected and positioned, particularly in relation to table cells and wrappers. The modifications aim to improve the precision of element selection and side menu rendering by adjusting event handling and element detection strategies, including updates to table creation functionalities. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
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: 0
🧹 Nitpick comments (1)
packages/editor/src/core/plugins/drag-handle.ts (1)
91-102: Add test coverage for table drag handle behavior.The changes look good but would benefit from additional test coverage to ensure the drag handle behavior works correctly with tables. Consider adding tests for:
- Table wrapper selection
- Preventing drag handles on table cell content
- Edge cases with nested draggable elements
Would you like me to help generate the test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/extensions/side-menu.tsx(0 hunks)packages/editor/src/core/plugins/drag-handle.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/core/extensions/side-menu.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/editor/src/core/plugins/drag-handle.ts (2)
91-94: LGTM! Prioritizing table wrapper selection.The new check correctly prioritizes table wrapper selection, preventing drag handles from being attached to inner table cell content.
100-102: Verify handling of valid draggable elements within tables.While the skip logic effectively prevents drag handles on table cell content, let's verify that this doesn't affect any valid draggable elements that might be intentionally placed within tables.
✅ Verification successful
Table drag handling implementation is correct
The skip logic for table cells is intentionally designed to:
- Allow dragging of entire tables as units
- Prevent disruption of table structure by blocking drag operations on individual cells
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for draggable elements within tables that might be affected. # Look for elements matching generalSelectors within table wrappers. echo "Searching for potentially affected draggable elements within tables..." # First, let's see what elements are typically placed within tables rg -l "table-wrapper.*(?:$(echo $generalSelectors | sed 's/, /|/g'))" # Then, check for any explicit drag-related attributes or classes within table contexts rg -l "table-wrapper.*draggable|drag"Length of output: 5823
Script:
#!/bin/bash # Check table and drag-related styles echo "=== Table CSS styles ===" cat packages/editor/src/styles/table.css echo -e "\n=== Drag-drop CSS styles ===" cat packages/editor/src/styles/drag-drop.css echo -e "\n=== Drag handle implementation ===" cat packages/editor/src/core/plugins/drag-handle.tsLength of output: 20343
Palanikannan1437
changed the title
Palanikannan1437
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/editor/src/styles/table.css (1)
Line range hint
21-27: Consider adding explicit CSS to prevent drag handles on cell content.While the backend changes handle drag functionality, adding CSS rules could provide an additional layer of prevention:
.table-wrapper table td, .table-wrapper table th { /* ... existing styles ... */ > * { margin-bottom: 0; + user-select: none; + pointer-events: none; } }This would ensure cell content cannot be dragged, reinforcing the changes made in the drag handle logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/styles/table.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-web
- GitHub Check: lint-space
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
packages/editor/src/styles/table.css (3)
19-19: LGTM! Padding adjustment helps with drag handle positioning.The reduction in horizontal padding from 20px to 10px helps maintain consistent spacing and prevents potential overlap with drag handles, aligning well with the PR's objective.
Line range hint
116-121: LGTM! Robust control handling prevents interference.The implementation properly handles control visibility during drag/resize operations through the
resize-cursorclass andcontrols--disabledstate, which helps prevent interference between dragging and control interactions.
Line range hint
1-121: Verify complete drag handle removal across related files.While the CSS changes support the objective, let's verify the complete implementation:
✅ Verification successful
Table drag handle removal is properly implemented
The implementation correctly handles table interactions by:
- Using dedicated column and row controls for tables
- Explicitly excluding table cells from drag handle functionality
- Maintaining proper CSS styling for table controls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related drag handle implementations echo "Searching for drag handle related files..." rg -l "drag-handle|dragHandle" --type ts --type tsx # Check for any remaining drag handle styles echo "Checking for additional drag handle styles..." rg -l "drag|draggable" --type css # Look for table-related components echo "Checking table-related components..." rg -l "table-wrapper|TableWrapper" --type ts --type tsxLength of output: 523
Script:
#!/bin/bash # Check content of drag-drop.css echo "=== Checking drag-drop.css content ===" cat packages/editor/src/styles/drag-drop.css # Search with correct typescript file extensions echo -e "\n=== Searching for drag handle related files ===" rg -l "drag-handle|dragHandle" -g "*.ts" -g "*.tsx" # Check table-related components echo -e "\n=== Checking table-related components ===" rg -l "table-wrapper|TableWrapper" -g "*.ts" -g "*.tsx"Length of output: 4007
Script:
#!/bin/bash # Check table-related implementations echo "=== Checking table view implementation ===" cat packages/editor/src/core/extensions/table/table/table-view.tsx echo -e "\n=== Checking table controls implementation ===" cat packages/editor/src/core/extensions/table/table/table-controls.ts echo -e "\n=== Checking drag handle implementation ===" cat packages/editor/src/core/plugins/drag-handle.tsLength of output: 32676
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/editor/src/core/extensions/table/table/utilities/create-cell.ts (1)
5-6: Consider using a more specific type for attrs.The implementation looks good, but consider using a more specific type for
attrsinstead ofRecord<string, any>to improve type safety and documentation.- attrs?: Record<string, any> + attrs?: { + colwidth?: number[]; + // add other valid attributes here + }packages/editor/src/core/extensions/table/table/table.ts (1)
116-118: Consider extracting default values to constants.The implementation looks good, but consider extracting magic numbers to named constants for better maintainability.
+const DEFAULT_TABLE_CONFIG = { + ROWS: 3, + COLS: 3, + COLUMN_WIDTH: 150, +} as const; + - ({ rows = 3, cols = 3, withHeaderRow = false, columnWidth = 150 } = {}) => + ({ rows = DEFAULT_TABLE_CONFIG.ROWS, cols = DEFAULT_TABLE_CONFIG.COLS, withHeaderRow = false, columnWidth = DEFAULT_TABLE_CONFIG.COLUMN_WIDTH } = {}) =>packages/editor/src/core/helpers/editor-commands.ts (1)
141-143: Consider extracting table options to a variable.While the implementation is correct, the table options could be extracted to improve readability and maintainability.
+ const tableOptions = { rows: 3, cols: 3, columnWidth: 150 }; + if (range) - editor.chain().focus().deleteRange(range).clearNodes().insertTable({ rows: 3, cols: 3, columnWidth: 150 }).run(); + editor.chain().focus().deleteRange(range).clearNodes().insertTable(tableOptions).run(); else - editor.chain().focus().clearNodes().insertTable({ rows: 3, cols: 3, columnWidth: 150 }).run(); + editor.chain().focus().clearNodes().insertTable(tableOptions).run();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/editor/src/core/extensions/table/table/table.ts(2 hunks)packages/editor/src/core/extensions/table/table/utilities/create-cell.ts(1 hunks)packages/editor/src/core/extensions/table/table/utilities/create-table.ts(1 hunks)packages/editor/src/core/helpers/editor-commands.ts(1 hunks)packages/editor/src/styles/table.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/styles/table.css
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
packages/editor/src/core/extensions/table/table/utilities/create-cell.ts (1)
9-9: LGTM! Consistent handling of attrs parameter.The implementation correctly uses the attrs parameter in both cell creation paths.
Also applies to: 12-12
packages/editor/src/core/extensions/table/table/utilities/create-table.ts (1)
19-19: LGTM! Consistent column width handling.The implementation correctly applies the column width to both regular and header cells, ensuring consistent behavior.
Also applies to: 26-26
packages/editor/src/core/extensions/table/table/table.ts (1)
42-47: LGTM! Well-typed interface extension.The Commands interface is properly extended to include the new columnWidth parameter.
packages/editor/src/core/helpers/editor-commands.ts (1)
Line range hint
1-1: Verify drag handle removal implementation.The PR objective is to remove drag handles from content within table cells, but the changes in these files focus on table structure and column width. Please verify if additional changes are needed in CSS or other files to achieve the drag handle removal.
packages/editor/src/core/extensions/table/table/utilities/create-table.ts
Show resolved
Hide resolved
3 participants
Description
This PR removes drag handles from the content within table cells
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Bug Fixes
Style
These changes refine the editor's table handling capabilities and enhance user interactions with table elements.