[WEB-5306] refactor: base layouts #8051
Conversation
- Introduced BASE_LAYOUTS constant for layout configuration. - Implemented LayoutSwitcher component for selecting layouts. - Added hooks for group drop targets and layout state management. - Created kanban and list components with group and item rendering. - Integrated drag-and-drop functionality for items within groups. - Updated translations for empty states and drag-and-drop messages.
…s in multiple languages
…d improve drag-and-drop validation - Modified GroupHeader to accept itemCount and onToggleGroup props. - Updated useGroupDropTarget to enhance drop validation logic for items. - Refactored group components to align with new IGroupHeaderProps interface.
…gleGroup usage in BaseKanbanGroup - Streamlined the onDrop prop definition in useGroupDropTarget for clarity. - Replaced local handleToggle function with direct onToggleGroup prop usage in BaseKanbanGroup.
… modes - Changed UseLayoutStateProps to a union type for better clarity on external and internal modes. - Simplified the useLayoutState hook to conditionally use external state and handlers based on the mode. - Ensured auto-scroll functionality respects the new props structure.
- Eliminated unnecessary comment regarding the use of external state/handlers in the useLayoutState hook for improved code clarity.
…apsedGroups and onToggleGroup - Updated BaseKanbanLayout and BaseListLayout to provide default values for collapsedGroups and onToggleGroup props. - Set mode to "external" in useLayoutState for both components to improve state management.
…apsedGroups and onToggleGroup - Modified BaseKanbanLayout and BaseListLayout to remove default values for collapsedGroups and onToggleGroup, making them required props. - Enhanced useLayoutState to conditionally set the mode based on the presence of external props, improving flexibility in layout management.
…actor/base-layouts
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes introduce conditional external/internal mode selection for layout state management in Kanban and List layout components, where external mode activates when both control props are provided, and refine type safety by requiring selectedLayout in LayoutSwitcher to be non-optional. Changes
Sequence DiagramsequenceDiagram
actor Parent
participant Layout as Kanban/List Layout
participant useLayoutState
Parent->>Layout: Render with props
alt External Mode (both props provided)
Layout->>Layout: useExternalMode = true
Layout->>useLayoutState: Pass externalCollapsedGroups<br/>& externalOnToggleGroup<br/>with mode: "external"
useLayoutState-->>Layout: Return external state
else Internal Mode (props missing)
Layout->>Layout: useExternalMode = false
Layout->>useLayoutState: No external props<br/>with mode: "internal"
useLayoutState-->>Layout: Return internal state
end
Layout-->>Parent: Render with selected mode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the layout components to support both internal and external state management for collapsed groups. The changes make the collapsedGroups and onToggleGroup props optional, allowing components to manage their own internal state when these props are not provided.
- Removed default values for
collapsedGroupsandonToggleGroupprops, making them truly optional - Added runtime checks to conditionally use external or internal mode based on whether props are provided
- Updated
selectedLayouttype inLayoutSwitcherto be non-nullable
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/web/core/components/base-layouts/list/layout.tsx | Implements conditional mode selection for useLayoutState based on presence of external props |
| apps/web/core/components/base-layouts/layout-switcher.tsx | Removes undefined union from selectedLayout prop type |
| apps/web/core/components/base-layouts/kanban/layout.tsx | Implements conditional mode selection for useLayoutState based on presence of external props |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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)
apps/web/core/components/base-layouts/kanban/layout.tsx (1)
27-38: Extract shared mode-switching logic to reduce duplication.This exact mode-switching pattern (lines 27-38) is duplicated in
apps/web/core/components/base-layouts/list/layout.tsx(lines 26-37). Consider extracting it into a shared utility function or custom hook to improve maintainability.Example:
const useExternalModeConfig = ( externalCollapsedGroups?: string[], externalOnToggleGroup?: (groupId: string) => void ) => { const useExternalMode = externalCollapsedGroups !== undefined && externalOnToggleGroup !== undefined; return useExternalMode ? { mode: "external" as const, externalCollapsedGroups, externalOnToggleGroup } : { mode: "internal" as const }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/core/components/base-layouts/kanban/layout.tsx(1 hunks)apps/web/core/components/base-layouts/layout-switcher.tsx(1 hunks)apps/web/core/components/base-layouts/list/layout.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/core/components/base-layouts/layout-switcher.tsx (1)
packages/types/src/base-layouts/base.ts (1)
TBaseLayoutType(55-55)
apps/web/core/components/base-layouts/kanban/layout.tsx (1)
apps/web/core/components/base-layouts/hooks/use-layout-state.ts (1)
useLayoutState(22-58)
apps/web/core/components/base-layouts/list/layout.tsx (1)
apps/web/core/components/base-layouts/hooks/use-layout-state.ts (1)
useLayoutState(22-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
apps/web/core/components/base-layouts/layout-switcher.tsx (1)
12-12: LGTM! Improved type safety.Making
selectedLayoutrequired prevents potential runtime errors and aligns with the component's actual usage, which never checks for undefined values.apps/web/core/components/base-layouts/kanban/layout.tsx (2)
23-24: LGTM! Clear prop naming for external control.The
externalprefix clearly indicates these props enable external control of collapse state, distinguishing them from internal state management.
27-38: Mode switching logic is correct, but consider edge case documentation.The logic correctly requires both
externalCollapsedGroupsandexternalOnToggleGroupto enable external mode. However, if a developer provides only one prop, it silently falls back to internal mode, which could be unexpected.Consider adding a development-mode warning or documenting this "all-or-nothing" requirement.
apps/web/core/components/base-layouts/list/layout.tsx (2)
20-21: LGTM! Consistent with kanban layout.The prop renaming and removal of defaults matches the pattern in
kanban/layout.tsx, providing a consistent API across layout types.
26-37: Mode switching logic is correct and consistent.The implementation mirrors the kanban layout's approach and correctly requires both external props for external mode. The same considerations about partial prop provision and code duplication noted in
kanban/layout.tsxapply here as well.
JayashTripathy
changed the title
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
…actor/base-layouts
Description
This pull request improves the handling of collapsed group state in both the Kanban and List base layouts by allowing external control of group collapse state and toggle behavior. The changes make it possible for parent components to manage collapsed groups and toggle actions, or to let the layouts manage them internally if not provided. Additionally, it updates the
LayoutSwitcherprop type for consistency.State management improvements for base layouts:
BaseKanbanLayoutandBaseListLayoutto accept optionalcollapsedGroupsandonToggleGroupprops, enabling external or internal state management for collapsed groups depending on whether these props are provided.Type consistency:
selectedLayoutprop inLayoutSwitcherto be required instead of optional, ensuring that a selected layout is always specified.Type of Change
Summary by CodeRabbit