feat: enhance canvas foundation capabilities #1055
Conversation
WalkthroughThis pull request introduces comprehensive changes to the canvas and materials management system, focusing on multi-selection functionality, node insertion, and component management. The modifications span multiple files across the canvas and materials packages, enhancing node interaction, keyboard handling, and component filtering. Key improvements include robust multi-node selection, expanded insertion logic for nodes, and more flexible component management through new utility functions. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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 (
|
github-actions
bot
added
enhancement
![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
🧹 Nitpick comments (11)
packages/canvas/container/src/components/CanvasAction.vue (3)
184-187: Add validation for multiStateLength prop.While the prop is correctly typed, adding validation would prevent potential issues with negative values.
multiStateLength: { type: Number, default: () => 0, + validator: (value) => value >= 0 },
243-256: Consider renaming showQuickAction for clarity.The computed property
showQuickActioncould be more descriptive, such asshowQuickActionForSingleNode, to better reflect its purpose.
Line range hint
1-735: Consider splitting component responsibilities.The component currently handles multiple concerns (selection, resize, quick actions, style calculations). Consider:
- Extracting the style calculation logic into a composable for better reusability
- Splitting the component into smaller, focused components (e.g.,
CanvasResize,CanvasQuickActions)This would improve maintainability and make the code more testable.
packages/plugins/materials/src/composable/useMaterial.js (1)
486-495: Consider enhancing error handling and type validation.While the function implementation is clean and efficient, consider adding:
- Type validation for
groupName- Explicit error handling for invalid group names
const getComponents = (components, groupName) => { if (!Array.isArray(components)) return [] + if (typeof groupName !== 'string') return [] + if (!groupName.trim()) return components return components.filter((item) => item.group === groupName) }packages/plugins/materials/src/meta/component/src/Main.vue (1)
91-98: Consider memoizing the initialization result.The
initComponentsfunction is well-implemented but could benefit from memoization to prevent unnecessary recalculations.+import { computed } from 'vue' -const initComponents = () => { +const initComponents = computed(() => { const groupName = panelState.materialGroup if (groupName) { return getComponents(components, groupName) } return components -} +})Then update the state initialization:
-components: initComponents(), +components: initComponents.value,packages/canvas/DesignCanvas/src/api/useCanvas.js (3)
264-271: Improve error handling for missing parent node.Instead of silently returning an empty object when the parent node is missing, consider throwing an error to help with debugging.
- if (!parentNode) { - return {} - } + if (!parentNode) { + throw new Error('Parent node not found') + }
291-308: Consider using a Map for position handling.The switch statement could be replaced with a Map for better maintainability and performance.
- switch (position) { - case 'before': - parentNode.children.unshift(newNodeData) - break - case 'out': - if (childrenNode) { - newNodeData.children = Array.isArray(childrenNode) ? [...childrenNode] : [childrenNode] - parentNode.children.splice(index, 1, newNodeData) - } - break - case 'bottom': - parentNode.children.splice(index + 1, 0, newNodeData) - break - default: - parentNode.children.push(newNodeData) - break - } + const positionHandlers = new Map([ + ['before', () => parentNode.children.unshift(newNodeData)], + ['out', () => { + if (childrenNode) { + newNodeData.children = Array.isArray(childrenNode) ? [...childrenNode] : [childrenNode] + parentNode.children.splice(index, 1, newNodeData) + } + }], + ['bottom', () => parentNode.children.splice(index + 1, 0, newNodeData)], + ['default', () => parentNode.children.push(newNodeData)] + ]) + const handler = positionHandlers.get(position) || positionHandlers.get('default') + handler()
351-352: Simplify array check using optional chaining.The array check can be simplified while maintaining safety.
- if (Array.isArray(nodeItem?.children) && nodeItem?.children.length) { + if (nodeItem?.children?.length) {packages/canvas/container/src/CanvasContainer.vue (3)
35-42: Ensure proper cleanup of theinsertContainerpanelThe insertion panel for adding a parent container is conditionally rendered based on
insertContainer. Ensure that this state is correctly managed to prevent the panel from remaining open unintentionally.Consider adding a watcher or method to handle the closure of the panel under specific conditions, such as when the user clicks outside the panel or completes the insertion.
213-214: Avoid unnecessary reassignment ofinsertContainerSetting
insertContainer.value = falseon everymousedownevent may interfere with the intended functionality.Review whether this reassignment is necessary here. If the goal is to close the insert container panel when clicking elsewhere, consider adding condition checks or handling this logic within a dedicated method.
229-229: Consistent handling ofinsertContainerin themouseupeventSimilar to the
mousedownevent, ensure that settinginsertContainer.value = falsealigns with the intended user experience.Confirm that this does not prematurely close the insert container panel during user interactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/canvas/DesignCanvas/src/api/useCanvas.js(2 hunks)packages/canvas/container/src/CanvasContainer.vue(11 hunks)packages/canvas/container/src/components/CanvasAction.vue(6 hunks)packages/canvas/container/src/components/CanvasMenu.vue(1 hunks)packages/canvas/container/src/container.js(6 hunks)packages/canvas/container/src/keyboard.js(6 hunks)packages/plugins/materials/src/composable/useMaterial.js(2 hunks)packages/plugins/materials/src/meta/component/src/Main.vue(2 hunks)packages/plugins/materials/src/meta/layout/src/Main.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (34)
packages/canvas/container/src/components/CanvasAction.vue (3)
12-12: LGTM! Improved condition for quick action visibility.The change from
resizetoshowQuickActionprovides better semantics and correctly handles multi-selection scenarios.
31-66: LGTM! Enhanced resize handle visibility logic.The resize handles are now correctly shown only for single node selection through the
showActioncomputed property.
420-423: LGTM! Added defensive null check.The null check for
docprevents potential runtime errors and follows defensive programming practices.packages/plugins/materials/src/meta/layout/src/Main.vue (2)
44-46: LGTM! Clean prop definition.The
groupNameprop is well-defined with appropriate type and default value.
55-55: LGTM! Proper state initialization.The
materialGroupis correctly initialized with the prop value in the panel state.packages/plugins/materials/src/composable/useMaterial.js (1)
525-526: LGTM! Clean export addition.The
getComponentsfunction is properly exported alongside other utility functions.packages/plugins/materials/src/meta/component/src/Main.vue (2)
57-57: LGTM! Clean function import.The
getComponentsfunction is properly destructured fromuseMaterial.
101-101: LGTM! Proper state initialization.The state is correctly initialized using the
initComponentsfunction.packages/canvas/DesignCanvas/src/api/useCanvas.js (1)
276-283: LGTM! Good error handling for reference node.The error handling for missing reference nodes is well implemented with a descriptive error message.
packages/canvas/container/src/container.js (6)
33-34: LGTM! Position constants updated correctly.The addition of the
OUTposition constant aligns with the new parent container functionality.
126-147: LGTM! Well-implemented state extraction.The
getMultiStatefunction properly extracts and validates node state with appropriate null checks.
170-181: LGTM! Efficient multi-selection handling.The
handleMultiStatefunction efficiently manages selection toggling using array methods.
380-393: LGTM! Clean implementation of container insertion.The
insertContainerfunction follows the same pattern as other insertion functions and properly handles ID generation.
816-818: LGTM! Proper integration of container insertion.The
insertNodefunction correctly handles the new 'out' position case.
149-168: Consider adding type validation for multiSelectedStates.The function assumes multiSelectedStates is a ref object with a value property.
packages/canvas/container/src/components/CanvasMenu.vue (1)
109-110: LGTM! Menu item properly integrated.The new container menu item is correctly added with appropriate code and value properties, maintaining consistency with existing menu items.
packages/canvas/container/src/CanvasContainer.vue (8)
99-101: InitializecontainerPanelandinsertContainercorrectlyThe references
containerPanelandinsertContainerare added to manage the parent container insertion panel. Verify that these are properly initialized and thatinsertContaineris reactive.The initialization looks correct. Ensure that any components or methods relying on these references handle them appropriately.
102-103: Confirm computed propertymultiStateLengthThe computed property
multiStateLengthcalculates the length ofmultiSelectedStates.This is a good approach to reactively track the number of selected states.
Line range hint
104-116: Verify the modifiedsetCurrentNodefunction for multi-selectionThe
setCurrentNodefunction now accepts adocparameter and includes logic for handling multi-selection withgetMultiStateandsetMultiState.Ensure that
getMultiStateandsetMultiStatefunctions are correctly implemented and that they effectively manage the multi-selection state.If these functions are defined in another file or context, confirm their integration here.
256-256: Register hotkey events consistentlyThe
registerHotkeyEvent(doc)call registers keyboard events within the iframe's document.Good implementation for capturing keyboard events in the iframe context.
279-279: IncludeinsertContainerin outside click handlingIn the
mousedownevent listener on thewindow, the check now includescontainerPanel.This ensures that clicks outside the
insertContainerpanel are handled correctly.
292-295: Implement the insertion logic for the 'out' positionWhen
position === 'out', theinsertContainerstate is updated accordingly.This enables the right-click functionality to add a parent container, aligning with the PR objectives.
303-311: WatchmultiStateLengthto manage the properties panelThe watcher on
multiStateLengthclears the properties panel when more than one state is selected.This is a suitable approach to prevent conflicting property displays during multi-selection.
2-14: Review the usage ofselectStatewithin thev-forloopThe
selectStateprop in<canvas-action>is conditionally set usingmultiStateLength > 1 ? multiState : selectState. This logic could lead to confusion or unintended behavior whenmultiStateLengthis exactly 1.Consider revising the condition to accurately reflect when multiple selections are involved. For instance:
- :selectState="multiStateLength > 1 ? multiState : selectState" + :selectState="multiStateLength >= 1 ? multiState : selectState"Alternatively, confirm that when
multiStateLengthis 1,selectStateshould indeed bemultiState.packages/canvas/container/src/keyboard.js (10)
13-17: Import and define new reactive states for multi-selectionThe imports and reactive references for
multiSelectedStatesandisCtrlPressedare added.This sets up the necessary state management for multi-selection and control key detection.
19-28: Define key codes forKEY_SandKEY_CTRLThe constants
KEY_SandKEY_CTRLare introduced for handling save shortcuts and control key presses.Defining key codes as constants improves code readability and maintainability.
48-53: RefactorhandlerDeleteto support multi-node deletionThe
handlerDeletefunction now iterates overmultiSelectedStatesto delete multiple nodes.This enhancement aligns with the multi-selection functionality, allowing users to delete multiple selected nodes simultaneously.
78-83: Implement save functionality withhandleSaveEventThe
handleSaveEventfunction is added to trigger the save action whenCtrl+Sis pressed.This provides a convenient shortcut for users to save their work, enhancing the user experience.
84-96: UpdatehandlerCtrlto includeKEY_Sfor savingThe control key handler now includes a case for
KEY_Sto invoke the save functionality.This integration ensures that pressing
Ctrl+Swill trigger the save action as expected.
116-130: Handle pasting multiple nodes from the clipboardThe new
handleMultiNodesPastefunction manages the pasting of multiple nodes when multiple nodes are selected.This addition enhances the clipboard functionality to work seamlessly with multi-selection.
157-162: Manage control key state withhandleKeyupEventThe
handleKeyupEventfunction updates theisCtrlPressedstate upon key release.This ensures accurate detection of the control key's state, which is important for multi-selection handling.
163-165: Remove hotkey events appropriatelyThe
removeHotkeyEventfunction now also removes thekeyupevent listener.Cleaning up event listeners helps prevent memory leaks and unintended behavior.
171-175: Register hotkey events includingkeyuplistenerThe
registerHotkeyEventfunction adds thekeyupevent listener to manage control key state.This ensures all necessary events are captured for the enhanced keyboard functionality.
181-181: Export new state variablesThe export statement now includes
multiSelectedStatesandisCtrlPressed.Exporting these variables allows other components to access and reactively use the multi-selection state and control key status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/canvas/container/src/keyboard.js (5)
28-30: Initialize the multiSelectedStates ref with an empty array.For better code clarity and to prevent potential undefined errors, initialize the ref:
-const multiSelectedStates = ref([]) +const multiSelectedStates = ref([]) // Initialize with empty array for multi-node selection
46-51: Add error handling for node removal operations.The current implementation might fail silently if node removal encounters an error. Consider adding try-catch blocks:
function handlerDelete() { - multiSelectedStates.value.forEach(({ id: schemaId }) => { - removeNodeById(schemaId) - }) + multiSelectedStates.value.forEach(({ id: schemaId }) => { + try { + removeNodeById(schemaId) + } catch (error) { + console.error(`Failed to remove node ${schemaId}:`, error) + } + }) multiSelectedStates.value.length = 0 }
76-93: Add user feedback for save operations.Consider adding visual feedback to inform users when the save operation succeeds or fails:
const handleSaveEvent = (event) => { const { openCommon } = getMetaApi(META_APP.Save) event.preventDefault() - openCommon() + try { + openCommon() + // Add toast/notification for successful save + } catch (error) { + console.error('Save failed:', error) + // Add toast/notification for failed save + } }
113-125: Optimize memory usage for large multi-node operations.The current implementation creates deep copies of all selected nodes, which could be memory-intensive for large selections. Consider:
- Adding a size limit for multi-selection
- Using a more memory-efficient copying mechanism
const handleMultiNodesPaste = (node, schema, parent) => { + const MAX_SELECTION = 50 // Prevent excessive memory usage + if (multiSelectedStates.value.length > MAX_SELECTION) { + console.warn(`Selection limit (${MAX_SELECTION}) exceeded. Some nodes will be skipped.`) + return + } if (multiSelectedStates.value.length === 1) { handleClipboardPaste(node, schema, parent) return } const selectedStates = multiSelectedStates.value.map(({ schema, parent }) => { return { node: copyObject(schema), schema: toRaw(schema), parent: toRaw(parent) } }) selectedStates.forEach(({ node, schema, parent }) => { handleClipboardPaste(node, schema, parent) }) }
154-168: Add JSDoc comments for the event registration functions.The rename from 'hostkey' to 'hotkey' is good, but the functions would benefit from clear documentation:
+/** + * Removes keyboard and clipboard event listeners from the specified DOM element. + * @param {HTMLElement} dom - The DOM element to remove events from + */ const removeHotkeyEvent = (dom) => { dom.removeEventListener('keydown', keyboardHandler) dom.removeEventListener('copy', handlerClipboardEvent) dom.removeEventListener('cut', handlerClipboardEvent) dom.removeEventListener('paste', handlerClipboardEvent) } +/** + * Registers keyboard and clipboard event listeners on the specified DOM element. + * @param {HTMLElement} dom - The DOM element to register events on + */ const registerHotkeyEvent = (dom) => { removeHotkeyEvent(dom) // ... rest of the function }packages/canvas/container/src/CanvasContainer.vue (3)
2-14: Optimize rendering performance and key uniqueness.The current implementation might have performance issues with large selections and potential key conflicts:
- Consider adding virtual scrolling for large selections
- Use a more unique key combining id with parent id:
- <div v-for="multiState in multiSelectedStates" :key="multiState.id"> + <div v-for="multiState in multiSelectedStates" :key="`${multiState.id}-${multiState.parent?.id}`">
303-311: Consider combining related watchers for better maintainability.The current implementation adds a separate watcher for multi-selection. Consider combining related watchers:
-watch( - () => multiStateLength.value, - (newVal) => { - if (newVal > 1) { - // 清空属性面板 - selectNode(null) - } - } -) +watch( + [() => multiStateLength.value, () => selectState.value], + ([length, state]) => { + if (length > 1) { + // Clear property panel for multi-selection + selectNode(null) + } else if (state) { + // Handle single selection + // Add your single selection logic here + } + } +)
292-295: Add type safety for position values.The current implementation uses string literals without type safety. Consider using an enum or constants:
+const INSERT_POSITIONS = { + OUT: 'out', + IN: 'in', + // Add other valid positions +} as const + const insertComponent = (position) => { - if (position === 'out') { + if (position === INSERT_POSITIONS.OUT) { insertContainer.value = position return } insertPosition.value = position }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/canvas/container/src/CanvasContainer.vue(11 hunks)packages/canvas/container/src/keyboard.js(7 hunks)
🔇 Additional comments (2)
packages/canvas/container/src/keyboard.js (1)
13-17: LGTM! Import statements are well-organized.The imports are appropriate for the multi-selection and keyboard shortcut functionality being added.
packages/canvas/container/src/CanvasContainer.vue (1)
203-211: LGTM! Multi-selection trigger implementation is correct.The implementation properly uses
event.ctrlKeyfor multi-selection, addressing the concerns from the previous review.
SonyLeo
changed the title
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
右键添加父级容器
在画布中右键页面元素 -> 添加父级 -> 容器：
选中指定 容器 点击 鼠标左键，
点击 画布元素 或点击 大纲树，即可查看新增的父容器
节点多选能力
点击 Ctrl + 鼠标左键 进行 多选节点 ，支持快捷键操作复制、粘贴 和 删除
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes