Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (1)
packages/editor/src/core/extensions/side-menu.tsx (1)
45-45: LGTM. Consider adding a comment explaining the change.The reduction of the
scrollThreshold.upvalue from 300 to 200 aligns with the PR objective of fixing drag handle scrolling. This change should make the side menu more responsive to upward scrolls.Could you provide more context on why 200 was chosen as the new threshold? Consider adding a brief comment explaining the rationale behind this specific value to improve maintainability.
For example:
scrollThreshold: { up: 200, // Reduced from 300 to improve responsiveness to upward scrolls down: 100 },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/editor/src/core/extensions/side-menu.tsx (1 hunks)
- packages/editor/src/core/plugins/drag-handle.ts (2 hunks)
🔇 Additional comments (3)
packages/editor/src/core/plugins/drag-handle.ts (3)
44-45: Addition of selectors for image components is appropriateThe inclusion of
.image-componentand.image-upload-componentin thegeneralSelectorsarray enhances the functionality to correctly identify image nodes at specific coordinates.
236-245:isScrollablefunction correctly determines scrollable elementsThe implementation of
isScrollableeffectively checks if an element is scrollable by examining theoverflowandoverflow-yCSS properties.
267-276:⚠️ Potential issueUse scrollable parent's dimensions instead of
window.innerHeightWhen calculating scroll thresholds for scrolling, using
window.innerHeightmay lead to incorrect behavior if thescrollableParentis not the window. To ensure accurate scrolling within nested scrollable containers, use the dimensions ofscrollableParent.Apply this diff to use
scrollableParent.getBoundingClientRect():267 if (e.clientY < scrollThreshold.up) { 268 const overflow = scrollThreshold.up - e.clientY; 269 const ratio = Math.min(overflow / scrollThreshold.up, 1); 270 const scrollAmount = -maxScrollSpeed * ratio; 271 scrollableParent.scrollBy({ top: scrollAmount }); 272 } else if (scrollableParent.getBoundingClientRect().bottom - e.clientY < scrollThreshold.down) { 273 const overflow = e.clientY - (scrollableParent.getBoundingClientRect().bottom - scrollThreshold.down); 274 const ratio = Math.min(overflow / scrollThreshold.down, 1); 275 const scrollAmount = maxScrollSpeed * ratio; 276 scrollableParent.scrollBy({ top: scrollAmount });To ensure this change works correctly, please verify the scroll behavior within nested scrollable elements.
Palanikannan1437
force-pushed
the
fix/drag-drop-scrolling
branch
from
ea8e17a to
10ce567
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/editor/src/core/plugins/drag-handle.ts (2)
236-245: Well-implementedisScrollablefunctionThe
isScrollablefunction is a good addition:
- It correctly checks both
overflowandoverflow-yproperties.- The type check at the beginning ensures type safety.
For a minor optimization, consider using
Array.prototype.some()instead of checking each property individually:return ["overflow", "overflow-y"].some(prop => ["auto", "scroll"].includes(style.getPropertyValue(prop)) );This change would make the function slightly more concise and efficient.
258-275: Improved drag event handling with dynamic scrollingThe changes to the drag event listener significantly enhance the drag-and-drop functionality:
- The use of
getScrollParentensures scrolling works correctly in nested scrollable containers.- The implementation handles both upward and downward scrolling smoothly.
- The scrolling speed is dynamically adjusted based on the cursor's proximity to the viewport edge, providing a natural feel.
To improve code readability, consider extracting the scrolling logic into a separate function:
const handleScroll = (clientY: number, scrollableParent: Element) => { const { innerHeight } = window; const { up, down } = options.scrollThreshold; if (clientY < up) { const ratio = Math.min((up - clientY) / up, 1); scrollableParent.scrollBy({ top: -maxScrollSpeed * ratio }); } else if (innerHeight - clientY < down) { const ratio = Math.min((clientY - (innerHeight - down)) / down, 1); scrollableParent.scrollBy({ top: maxScrollSpeed * ratio }); } }; // In the drag event listener: handleScroll(e.clientY, scrollableParent);This refactoring would make the main drag event listener more concise and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/editor/src/core/extensions/side-menu.tsx (1 hunks)
- packages/editor/src/core/plugins/drag-handle.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/side-menu.tsx
🔇 Additional comments (4)
packages/editor/src/core/plugins/drag-handle.ts (4)
33-45: ImprovednodeDOMAtCoordsfunction with extended functionalityThe changes to the
nodeDOMAtCoordsfunction are well-implemented:
- Exporting the function enhances its reusability across the codebase.
- The addition of
.image-componentand.image-upload-componentselectors extends the function's capability to identify image-related components, which is likely beneficial for drag-and-drop operations involving images.These improvements contribute to a more versatile and robust drag-and-drop system.
247-256: Well-implementedgetScrollParentfunctionThe
getScrollParentfunction is a valuable addition:
- It efficiently traverses the DOM tree to find the nearest scrollable parent.
- The function correctly uses the
isScrollablehelper function.- The fallback to
document.scrollingElementordocument.documentElementensures a valid scrollable element is always returned.This function will be crucial for implementing smooth scrolling behavior during drag operations.
251-251: Removal of console.log statementThe removal of the
console.logstatement is appropriate:
- It eliminates unnecessary logging in production code.
- This change addresses the previous review comment suggesting its removal.
Good job on maintaining clean code by removing debugging statements.
Line range hint
1-365: Overall assessment: Significant improvements to drag-and-drop functionalityThis pull request introduces valuable enhancements to the drag-and-drop functionality:
- The
nodeDOMAtCoordsfunction has been expanded to handle image components.- New utility functions
isScrollableandgetScrollParenthave been added, improving the code's ability to handle scrollable containers.- The drag event handling now includes dynamic scrolling, greatly improving the user experience when dragging elements near viewport edges.
- Debugging code has been removed, keeping the production code clean.
These changes collectively contribute to a more robust, user-friendly drag-and-drop system. The code is well-structured and thoughtfully implemented. Minor suggestions for optimization and readability have been provided in the individual comments.
Great work on this feature enhancement!
* fix: drag handle scrolling fixed * fix: closest scrollable parent found and scrolled * fix: removed overflow auto from framerenderer * fix: make dragging dynamic and smoother
4 participants
Description
Added dynamic scrolling in Pages such that you could scroll easily while dragging and dropping nodes.
Summary by CodeRabbit