fix(DragDropSort): support custom drag button aria-label and fix falsy id overlay#12417
fix(DragDropSort): support custom drag button aria-label and fix falsy id overlay#12417logonoff wants to merge 1 commit intopatternfly:mainfrom
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an optional ChangesDrag-Drop Accessibility and Correctness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Preview: https://pf-react-pr-12417.surge.sh A11y report: https://pf-react-pr-12417-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx (1)
112-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissed falsy-
activeIdcheck incollisionDetectionStrategy— same bug as lines 243/293.Line 114 still uses
activeId &&, which short-circuits to0(falsy) whenactiveIdis the numeric value0. This means theclosestCenterpath for container-level collision detection is skipped for any drag operation whereactiveId === 0, breaking multi-container sorting for items with that ID — the exact scenario this PR targets.🐛 Proposed fix
- if (activeId && activeId in items) { + if (activeId != null && activeId in items) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx` around lines 112 - 119, The collisionDetectionStrategy callback incorrectly uses "activeId && activeId in items", which treats numeric 0 as falsy and skips the closestCenter branch; change that check to a null/undefined-safe test (e.g. activeId != null or activeId !== null && activeId !== undefined) so 0 is treated as a valid activeId, then proceed to call closestCenter with the filtered droppableContainers as before (see collisionDetectionStrategy, activeId, items, closestCenter, and args.droppableContainers.filter).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx`:
- Around line 112-119: The collisionDetectionStrategy callback incorrectly uses
"activeId && activeId in items", which treats numeric 0 as falsy and skips the
closestCenter branch; change that check to a null/undefined-safe test (e.g.
activeId != null or activeId !== null && activeId !== undefined) so 0 is treated
as a valid activeId, then proceed to call closestCenter with the filtered
droppableContainers as before (see collisionDetectionStrategy, activeId, items,
closestCenter, and args.droppableContainers.filter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a459f5fa-c8ef-42c2-80a8-23f6f7a85b7f
📒 Files selected for processing (2)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsxpackages/react-drag-drop/src/components/DragDrop/Draggable.tsx
logonoff
force-pushed
the
12416-dnd
branch
2 times, most recently
from
c6d9241 to
6e11dbd
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx (1)
147-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissed falsy-
0fix onlastOverId.current.This is the same bug class the PR is addressing. When
lastOverId.current === 0, the truthy guard short-circuits to[]instead of returning[{ id: 0 }], breaking the collision-detection fallback for items with numeric ID0.🐛 Proposed fix
- return lastOverId.current ? [{ id: lastOverId.current }] : []; + return lastOverId.current != null ? [{ id: lastOverId.current }] : [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx` at line 147, The return uses a truthy check that drops numeric id 0; update the guard in the function inside DragDropContainer (the expression returning lastOverId.current ? [{ id: lastOverId.current }] : []) to explicitly check for null/undefined (e.g., lastOverId.current != null) so that 0 is treated as a valid id and the collision-detection fallback returns [{ id: 0 }] when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx`:
- Line 147: The return uses a truthy check that drops numeric id 0; update the
guard in the function inside DragDropContainer (the expression returning
lastOverId.current ? [{ id: lastOverId.current }] : []) to explicitly check for
null/undefined (e.g., lastOverId.current != null) so that 0 is treated as a
valid id and the collision-detection fallback returns [{ id: 0 }] when
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07404410-6d19-4d51-80d8-3e80609e1200
📒 Files selected for processing (2)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsxpackages/react-drag-drop/src/components/DragDrop/Draggable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-drag-drop/src/components/DragDrop/Draggable.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx (1)
171-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete fix:
!overIdstill silently drops drag-over events for items with id0
handleDragOverguards with!overIdon line 171, which short-circuits and returns early whenoverId === 0. If a consumer passesDraggableObject.id = 0, dragging over that item (or a container keyed0) will be silently ignored, preventing container-move events from firing and leaving the item stuck. The identical falsy-id problem the PR fixes foractiveIdis still present here foroverId.🐛 Proposed fix
- if (!overId || activeId in items) { + if (overId == null || activeId in items) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx` around lines 171 - 173, The guard in handleDragOver incorrectly uses "!overId" which treats 0 as falsy and ignores valid IDs; change the check to explicitly test for null/undefined (e.g., "overId == null" or "typeof overId === 'undefined'") so ids of 0 are handled correctly, matching the existing fix pattern used for activeId and ensuring container-move events fire for items keyed with 0; update the conditional that currently reads "if (!overId || activeId in items) { return; }" to use the explicit null/undefined check for overId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx`:
- Around line 171-173: The guard in handleDragOver incorrectly uses "!overId"
which treats 0 as falsy and ignores valid IDs; change the check to explicitly
test for null/undefined (e.g., "overId == null" or "typeof overId ===
'undefined'") so ids of 0 are handled correctly, matching the existing fix
pattern used for activeId and ensuring container-move events fire for items
keyed with 0; update the conditional that currently reads "if (!overId ||
activeId in items) { return; }" to use the explicit null/undefined check for
overId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cecdc4e-0fc3-42ce-ae64-ef933a90c6fb
📒 Files selected for processing (2)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsxpackages/react-drag-drop/src/components/DragDrop/Draggable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-drag-drop/src/components/DragDrop/Draggable.tsx
…y id overlay Adds a `dragButtonAriaLabel` prop to `Draggable` that forwards to `DragButton`, allowing consumers to provide a translated accessible name for the drag handle via `DraggableObject.props`. Also fixes `DragDropContainer` rendering the literal text "0" as the drag overlay when a draggable item has a falsy numeric id (e.g. `0`). The `activeId && getDragOverlay()` expression short-circuits to `0` which React renders as text. Changed to `activeId != null`.
2 participants
What: Closes #12416
dragButtonAriaLabelprop toDraggablethat forwards toDragButton, allowing consumers to provide a translated accessible name for the drag handle viaDraggableObject.props.DragDropContainerrendering the literal text"0"as the drag overlay when a draggable item has a falsy numeric id (e.g.0). TheactiveId && getDragOverlay()expression short-circuits to0which React renders as text. Changed toactiveId != null.Additional issues: N/A
Summary by CodeRabbit
Bug Fixes
New Features