✨ Upgrade @dnd-kit/sortable, refactor migration-targets #2406
Conversation
sjd78
force-pushed
the
migration_targets_dnd
branch
from
ec32bc4 to
8fe38e4
Compare
sjd78
force-pushed
the
migration_targets_dnd
branch
from
8fe38e4 to
c9cbe64
Compare
client/src/app/pages/migration-targets/components/dnd/sortable-target-item.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/migration-targets/components/dnd/sortable-target-item.tsx
Show resolved
Hide resolved
client/src/app/pages/migration-targets/components/dnd/target-item.tsx
Outdated
Show resolved
Hide resolved
sjd78
force-pushed
the
migration_targets_dnd
branch
from
c9cbe64 to
0b4ac83
Compare
WalkthroughThe drag-and-drop system for migration targets was refactored. Several DnD-related components and CSS files were removed and replaced with new, type-safe components and updated styling. The drag overlay now uses full target data, and the UI layout uses PatternFly's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MigrationTargets
participant SortableTargetItem
participant TargetItem
participant TargetCard
User->>MigrationTargets: Drag target item
MigrationTargets->>SortableTargetItem: Render with target data
SortableTargetItem->>TargetItem: Render with drag handle and props
TargetItem->>TargetCard: Render target info, actions, and drag handle
User-->>MigrationTargets: Drop target item
MigrationTargets->>SortableTargetItem: Update position/order
Poem
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 Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (6)
✨ Finishing Touches
🪧 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 (
|
![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 (3)
client/src/app/pages/migration-targets/components/dnd/target-item.tsx (1)
46-46: Consider updating displayName for clarity.The
displayNameis set to "Item" which might be confusing since the component is namedTargetItem. Consider updating it to match the component name for better debugging experience.-TargetItem.displayName = "Item"; +TargetItem.displayName = "TargetItem";client/src/app/pages/migration-targets/migration-targets.tsx (2)
272-272: Consider moving inline style to CSS for better maintainability.The inline
paddingBlockStart: 0style could be moved to a CSS class for better maintainability and consistency with the rest of the codebase.- <PageSection style={{ paddingBlockStart: 0 }}> + <PageSection className="migration-targets-dnd-section">Add to your CSS file:
.migration-targets-dnd-section { padding-block-start: 0; }
286-290: Consider making the fixed height more flexible.The explicit
height: "410px"might cause layout issues if target card content varies significantly in size. Consider using a more flexible approach likeminHeightor CSS classes.- style={{ height: "410px" }} + style={{ minHeight: "410px" }}Or better yet, move to a CSS class:
- style={{ height: "410px" }} + className="sortable-target-item"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
client/package.json(1 hunks)client/src/app/components/target-card/target-card.css(0 hunks)client/src/app/components/target-card/target-card.tsx(4 hunks)client/src/app/pages/migration-targets/components/dnd/grid.css(0 hunks)client/src/app/pages/migration-targets/components/dnd/grid.tsx(0 hunks)client/src/app/pages/migration-targets/components/dnd/item.tsx(0 hunks)client/src/app/pages/migration-targets/components/dnd/sortable-item.tsx(0 hunks)client/src/app/pages/migration-targets/components/dnd/sortable-target-item.tsx(1 hunks)client/src/app/pages/migration-targets/components/dnd/target-item.css(1 hunks)client/src/app/pages/migration-targets/components/dnd/target-item.tsx(1 hunks)client/src/app/pages/migration-targets/migration-targets.css(0 hunks)client/src/app/pages/migration-targets/migration-targets.tsx(9 hunks)
💤 Files with no reviewable changes (6)
- client/src/app/pages/migration-targets/migration-targets.css
- client/src/app/components/target-card/target-card.css
- client/src/app/pages/migration-targets/components/dnd/grid.tsx
- client/src/app/pages/migration-targets/components/dnd/grid.css
- client/src/app/pages/migration-targets/components/dnd/item.tsx
- client/src/app/pages/migration-targets/components/dnd/sortable-item.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
client/src/app/pages/migration-targets/components/dnd/target-item.tsx (2)
client/src/app/api/models.ts (1)
Target(485-495)client/src/app/components/target-card/target-card.tsx (1)
TargetCard(54-216)
client/src/app/pages/migration-targets/components/dnd/sortable-target-item.tsx (2)
client/src/app/api/models.ts (1)
Target(485-495)client/src/app/pages/migration-targets/components/dnd/target-item.tsx (1)
TargetItem(17-44)
client/src/app/components/target-card/target-card.tsx (1)
client/src/app/components/KebabDropdown.tsx (1)
KebabDropdown(16-44)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-test
- GitHub Check: test-image-build (arm64)
- GitHub Check: test-image-build (amd64)
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (11)
client/src/app/pages/migration-targets/components/dnd/target-item.css (1)
1-8: Well-implemented drag cursor styles.The CSS follows standard UX patterns for drag-and-drop interactions with clear cursor feedback. The
.grabbableclass naming is semantic and the hover/active states provide appropriate visual cues.client/package.json (1)
24-24: Verify major version upgrade compatibility.The upgrade from
@dnd-kit/sortablev7 to v10 is a significant jump that likely introduces breaking changes. Ensure that all drag-and-drop functionality has been thoroughly tested with the new version and that the implementation correctly handles any API changes.#!/bin/bash # Description: Check for any potential issues with the @dnd-kit/sortable v10 upgrade # Expected: Find all usages of @dnd-kit/sortable hooks and verify they align with v10 API echo "=== Searching for @dnd-kit/sortable imports and usage ===" rg -A 3 "@dnd-kit/sortable" echo "=== Searching for useSortable hook usage ===" rg -A 10 "useSortable"client/src/app/pages/migration-targets/components/dnd/target-item.tsx (1)
1-44: Well-structured drag-and-drop component with good separation of concerns.The implementation correctly uses
forwardRef, properly handles drag-and-drop props, and provides good accessibility support with appropriate ARIA labels. The separation between drag functionality and target rendering is clean.client/src/app/pages/migration-targets/components/dnd/sortable-target-item.tsx (1)
1-47: Excellent implementation of sortable drag-and-drop wrapper.The component correctly uses the
useSortablehook from the upgraded@dnd-kit/sortablev10, properly handles transform and transition styling, and cleanly forwards all necessary props to theTargetItemcomponent. The implementation follows the official documentation patterns.client/src/app/components/target-card/target-card.tsx (2)
43-43: Excellent refactoring to externalize drag handle.The change from internal drag handle management to accepting an external
dndSortHandleprop improves separation of concerns and makes the component more reusable. This allows parent components to control the drag handle appearance and behavior while keeping the target card focused on its core presentation logic.Also applies to: 61-61, 150-150
152-164: Clean reordering of conditional rendering logic.The reordering of the custom target label and dropdown rendering logic maintains the same functionality while improving code readability. The logic flow is now more intuitive.
client/src/app/pages/migration-targets/migration-targets.tsx (5)
73-73: Excellent improvement to drag state management.Storing the full
Targetobject instead of just the ID improves type safety and eliminates the need for subsequent lookups. This change enhances the drag-and-drop implementation.
178-186: Clean and correct drag event handler implementation.The updated handlers properly implement the new approach of storing full
Targetobjects. The null handling inhandleDragStartis appropriate, and the simplifiedhandleDragEndaligns well with the new state management strategy.
284-284: Good use of PatternFly's Gallery component.The replacement of the custom grid with PatternFly's
Gallerycomponent with responsiveminWidthsandhasGutterprops aligns well with design system best practices.
302-302: Excellent improvement to drag overlay.Using the full
Targetobject in the drag overlay instead of just an ID provides much richer visual feedback during drag operations. This enhances the user experience significantly.
22-23: ```shell
#!/bin/bashCheck exports and definitions in component files
echo "===== SortableTargetItem Exports and Usage ====="
grep -n "export" client/src/app/pages/migration-targets/components/dnd/sortable-target-item.tsx || true
grep -n "SortableTargetItem" client/src/app/pages/migration-targets/components/dnd/sortable-target-item.tsx || trueecho ""
echo "===== TargetItem Exports and Usage ====="
grep -n "export" client/src/app/pages/migration-targets/components/dnd/target-item.tsx || true
grep -n "TargetItem" client/src/app/pages/migration-targets/components/dnd/target-item.tsx || true</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
sjd78
force-pushed
the
migration_targets_dnd
branch
from
160a509 to
7c09c8e
Compare
Upgrade `@dnd-kit` and `@dnd-kit/sortable` to latest. Refactor the migration-targets page and target card components. Simplified the components for more streamlined use on the custom migration targets page and on the analysis wizard target selection step. The migration-targets page layout updated to use `Gallery` instead of the custom grid. Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
sjd78
force-pushed
the
migration_targets_dnd
branch
from
7c09c8e to
f5b7b80
Compare
Upgrade `@dnd-kit` and `@dnd-kit/sortable` to latest. Refactor the migration-targets page and target card components. Simplified the components for more streamlined use on the custom migration targets page and on the analysis wizard target selection step. The migration-targets page layout updated to use `Gallery` instead of the custom grid. Signed-off-by: Scott J Dickerson <[email protected]>
2 participants
Upgrade
@dnd-kitand@dnd-kit/sortableto latest.Refactor the migration-targets page and target card components. Simplified the components for more streamlined use on the custom migration targets page and on the analysis wizard target selection step. The migration-targets page layout updated to use
Galleryinstead of the custom grid.Summary by CodeRabbit
New Features
Refactor
Style
Chores