feat(page-editor): add visual feedback during page reordering#6289
feat(page-editor): add visual feedback during page reordering#6289betilloXann wants to merge 2 commits intoStirling-Tools:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Improves page reordering UX in the page editor by keeping the page index indicator visible when a thumbnail is hovered or when another page is dragged over it.
Changes:
- Add
useDndContext()usage to detect the current drag-over target. - Show the page number indicator on hover or drag-over by adjusting opacity.
- Increase the indicator
zIndexto keep it visible during drag interactions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { over } = useDndContext(); | ||
| const isOver = over?.id === page.id; | ||
|
|
There was a problem hiding this comment.
useDndContext() inside every PageThumbnail will subscribe each thumbnail to DnD context updates (e.g., over changes during pointer move). This can cause many thumbnails to re-render on every drag movement, undermining the memoization/virtualization work in DragDropGrid and potentially hurting performance on large PDFs. Prefer passing the current drop target id (or a per-item isOver boolean from useDroppable) down via props so only the relevant items update.
| zIndex: 2, | ||
| opacity: 0, | ||
| zIndex: 20, | ||
| opacity: isHovered || isOver ? 0.6 : 0, |
There was a problem hiding this comment.
isOver is derived from over?.id === page.id, but in this grid each item is both draggable and droppable, so over.id can be the active dragged item itself. That can make the page number appear on the dragged thumbnail rather than the intended drop target, and it may also disagree with DragDropGrid’s cursor-based hoveredItemId (which explicitly excludes the active id and drives the actual reorder target). Consider driving this UI from the same drop-target state used for reordering (e.g., hoveredItemId) or at least ensuring the dragged item never counts as isOver.
| opacity: isHovered || isOver ? 0.6 : 0, | |
| opacity: isHovered || (isOver && !isDragging) ? 0.6 : 0, |
- Moved drag and drop state from PageThumbnail to DragDropGrid to avoid re-rendering all thumbnails while dragging. - Updated PageThumbnail to get `isOverTarget` from props. - Fixed the logic so the page number does not show on the item being dragged. - Fixed comments from the Copilot PR review.
|
Applied fixes from Copilot review. Moved the drag and drop state to the Grid component to avoid a possible performance problem and fixed the indicator bug on the dragged item. Ready for review. |
Description of Changes
What was changed
hoveredItemId) toDragDropGridto improve performance (this avoids re-rendering all thumbnails while dragging).isOverTargetas a prop toPageThumbnailto control the indicator.opacityto0.6when a page is hovered or when another page is dragged over it (not for the page being dragged).zIndexto20so the indicator stays visible during drag.Why the change was made
Future Improvements (Optional)
This PR fixes the visibility issue. In the future, we could show the exact target position (for example, “Move to position 5”) if this is useful.
UI Changes (if applicable)
Related to #6170
Checklist
General
UI Changes (if applicable)
Testing
task checkto verify linters and tests pass.