feat: outline tree support dragging #1050
Conversation
…to feat/outline-tree-enhanced
…to feat/outline-tree-enhanced
WalkthroughThe pull request introduces two significant changes: first, it expands the canvas API in Changes
Suggested labels
Suggested reviewers
Poem
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 (
|
gene9831
changed the title
…to feat/outline-tree-enhanced
![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
🔭 Outside diff range comments (1)
packages/canvas/container/src/container.js (1)
Line range hint
668-679: Revise the logic in theallowInsertfunction to correctly handle insertion rules.The current implementation of
allowInsertmay not correctly evaluate the insertion permissions when bothchildWhitelistanddescendantBlacklistare specified. The variableflagis being overwritten, which can lead to unintended behavior.Consider updating the logic to properly account for both whitelist and blacklist conditions without overwriting the previous state. Here's a suggested revision:
export const allowInsert = (configure = hoverState.configure || {}, data = dragState.data || {}) => { const { nestingRule = {} } = configure const { childWhitelist = [], descendantBlacklist = [] } = nestingRule // The parent node must be a container to allow insertion if (!configure.isContainer) { return false } - let flag = true - // Whitelist - flag = childWhitelist.length ? childWhitelist.includes(data?.componentName) : true - - // Blacklist - if (descendantBlacklist.length) { - flag = !descendantBlacklist.includes(data?.componentName) - } + // Check child whitelist + if (childWhitelist.length && !childWhitelist.includes(data?.componentName)) { + return false + } + + // Check descendant blacklist + if (descendantBlacklist.length && descendantBlacklist.includes(data?.componentName)) { + return false + } + + return true }This change ensures that:
- If a
childWhitelistis defined, only components in the whitelist are allowed.- If a
descendantBlacklistis defined, components in the blacklist are disallowed.- Both conditions are evaluated independently, and any disallowing condition immediately returns
false.
🧹 Nitpick comments (1)
packages/plugins/tree/src/DraggableTree.vue (1)
160-192: Correct the spelling of 'flatternNode' to 'flattenNode'.The function name
flatternNodeis misspelled. The correct spelling isflattenNode. This will improve code readability and maintain consistency.Apply this diff to correct the spelling:
-const flatternNode = (node, parentId, level = 0, collapsed = false) => { +const flattenNode = (node, parentId, level = 0, collapsed = false) => { - .map((child) => flatternNode(child, node.id, level + 1, collapsed || collapseMap.value[node.id])) + .map((child) => flattenNode(child, node.id, level + 1, collapsed || collapseMap.value[node.id])) -return [rowItem].concat(descendantNodes) +return [rowItem, ...descendantNodes]Also, update all occurrences of
flatternNodeandflatternNodes:-const flatternNodes = (nodes) => { - const dummyNode = { children: nodes } - return flatternNode(dummyNode, null, -1).slice(1) +const flattenNodes = (nodes) => { + const dummyNode = { children: nodes } + return flattenNode(dummyNode, null, -1).slice(1) } -const rows = computed(() => flatternNodes(normalizedData.value)) +const rows = computed(() => flattenNodes(normalizedData.value))Also applies to: 189-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/canvas/container/src/container.js(1 hunks)packages/plugins/tree/src/DraggableTree.vue(1 hunks)packages/plugins/tree/src/Main.vue(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/plugins/tree/src/Main.vue (1)
26-26:⚠️ Potential issueFix the syntax error in the class binding on the
<span>element.There's an extra closing square bracket in the
:classdirective on line 26, which will cause a syntax error and prevent the component from rendering correctly.Apply this diff to fix the syntax error:
- <span :class="['row-label', { 'node-isblock': row.rawData.componentType === 'Block' }]}">{{ + <span :class="['row-label', { 'node-isblock': row.rawData.componentType === 'Block' }]">{{Likely invalid or redundant comment.
…to feat/outline-tree-enhanced
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/plugins/tree/src/Main.vue (2)
23-35: Consider adding aria-label for accessibility.The row content template should include accessibility attributes for the interactive elements.
Add aria-labels to improve accessibility:
- <svg-icon v-if="eyeOpen(row.id)" name="eye" @mouseup="showNode(row.rawData)"></svg-icon> - <svg-icon v-if="!eyeOpen(row.id)" name="eye-invisible" @mouseup="showNode(row.rawData)"></svg-icon> + <svg-icon v-if="eyeOpen(row.id)" name="eye" @mouseup="showNode(row.rawData)" aria-label="Hide node"></svg-icon> + <svg-icon v-if="!eyeOpen(row.id)" name="eye-invisible" @mouseup="showNode(row.rawData)" aria-label="Show node"></svg-icon>
206-209: Add fallback for missing material icons.The
getIconNamefunction should handle cases wheregetMaterialreturns undefined.Add null check and fallback:
const getIconName = (row) => { - const iconName = getMaterial(row.rawData.componentName).icon || 'plugin-icon-page' + const material = getMaterial(row.rawData.componentName) + const iconName = material?.icon || 'plugin-icon-page' return iconName.toLowerCase() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/tree/src/Main.vue(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/plugins/tree/src/Main.vue (4)
12-22: LGTM! The DraggableTree component setup looks good.The component is well-configured with appropriate props and event handlers for tree functionality.
41-41: LGTM! Import statements are properly organized.The imports are well-structured, with Vue utilities and local components clearly separated.
Also applies to: 47-47
167-195: LGTM! Comprehensive drag and drop handling.The drag and drop implementation includes proper validation and edge case handling.
233-265: LGTM! Well-structured styles with proper theming.The styles make good use of CSS variables and flex layout, with proper overflow handling and hover interactions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/plugins/tree/src/Main.vue (1)
162-165:⚠️ Potential issueAdd error handling for external API calls
The
getConfigureandallowInsertcalls from the canvas API should include error handling to prevent potential runtime errors.Apply this diff to add error handling:
const { getConfigure, allowInsert } = useCanvas().canvasApi.value + try { return !allowInsert(getConfigure(dropTo.rawData.componentName), dragged.rawData) + } catch (error) { + console.error('Failed to validate drop operation:', error) + return true + }This ensures that any exceptions are caught, and appropriate fallback behavior is provided.
🧹 Nitpick comments (2)
packages/plugins/tree/src/DraggableTree.vue (2)
246-249: Fix the comment to match the code logicThe comment on line 248 incorrectly states
// 顶部边框(top border), but the code returns{ position: 'bottom', ... }. The comment should reflect that it's handling the bottom border.Apply this diff to correct the comment:
if (offsetY >= rect.height - threshold) { - // 顶部边框 + // 底部边框 return { position: 'bottom', borderClass: 'border-bottom' } }
295-308: Simplify coordinate checks inhandleDragLeaveContainerThe coordinate checks in
handleDragLeaveContainercould be refactored for better readability and maintainability.Consider creating a helper function to handle the boundary checks or simplifying the conditions. This can make the code cleaner and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/plugins/tree/src/DraggableTree.vue(1 hunks)packages/plugins/tree/src/Main.vue(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/plugins/tree/src/DraggableTree.vue (3)
9-9: Remove leading space in class name to prevent unintended CSS classIn the class array, there's a leading space in
' flex-center', which will result in a class name with a leading space. This could cause the CSS class not to apply correctly.Apply this diff to fix the class name:
- ' flex-center', + 'flex-center',
225-226: Translate comments to English for consistencyThere are comments written in Chinese in the code. For better maintainability and to ensure all team members can understand and contribute, please translate comments to English.
Example:
- // 去掉ghost image + // Remove ghost imagePlease update the other comments accordingly.
Also applies to: 242-244, 247-249, 252-253, 260-261, 263-266, 279-281, 283-285, 292-295, 297-307
319-322: Avoid using the universal selector '*' to improve performanceUsing the universal selector
*can negatively impact performance as it applies styles to all elements. Consider specifying more targeted selectors to limit the scope.Apply this diff to modify the selector:
- &, - * { + & { cursor: pointer; }Alternatively, target specific child elements that require the
cursor: pointerstyle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/tree/src/DraggableTree.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
| import { useCanvas, useMaterial, useLayout, useMessage } from '@opentiny/tiny-engine-meta-register' | ||
| import { extend } from '@opentiny/vue-renderless/common/object' | ||
| import { typeOf } from '@opentiny/vue-renderless/common/type' | ||
| import DraggableTree from './DraggableTree.vue' |
There was a problem hiding this comment.
DraggableTree 后续可以考虑放到公共组件中（抽离是否允许拖放等逻辑给上层控制之后）
…to feat/outline-tree-enhanced
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/plugins/tree/src/DraggableTree.vue (6)
26-26: Consider optimizing the indentation implementation.Instead of using padding for indentation, consider using a width calculation to reduce DOM nodes:
- <div class="content flex-center" :style="{ paddingLeft: `${12 * row.level}px` }"> + <div class="content flex-center"> + <span class="indent" :style="{ width: `${12 * row.level}px` }"></span>
159-181: Consider simplifying the flattening logic.The current implementation could be simplified using Array.prototype.flat:
- return [rowItem].concat(descendantNodes) + return [rowItem, ...descendantNodes]
26-26: Consider optimizing the indentation implementation.The current approach of using padding for indentation could lead to performance issues with deeply nested trees. Consider using CSS transforms or a more efficient approach.
Apply this diff to optimize the indentation:
- <div class="content flex-center" :style="{ paddingLeft: `${12 * row.level}px` }"> + <div class="content flex-center" :class="`indent-level-${row.level}`">And add these styles:
.draggable-tree { @for $i from 0 through 10 { .indent-level-$(i) { padding-left: $(i * 12)px; } } }
85-90: Consider enhancing type definitions.The JSDoc type definitions could be more comprehensive to improve code maintainability.
Add more detailed type information:
/** * @typedef {Object} Node * @property {string} id - Unique identifier for the node * @property {string} label - Display label for the node - * @property {Node[]} [children] + * @property {Node[]} [children] - Optional array of child nodes - * @property {any} rawData + * @property {Object} rawData - Original data object passed to the component */ /** * @typedef {Object} RowItem * @property {string} id - Unique identifier for the row * @property {string} label - Display label for the row * @property {number} level - Tree depth level (0 for root nodes) * @property {string} [parentId] - ID of the parent node * @property {RowItem} parent - Reference to parent row item * @property {boolean} hasChildren - Whether the row has child nodes - * @property {collapsed} boolean + * @property {boolean} collapsed - Whether the node is collapsed - * @property {any} rawData + * @property {Object} rawData - Original data object passed to the component */Also applies to: 140-149
219-233: Enhance drag start error handling.The drag start handler could benefit from additional error handling and validation.
Apply this diff to improve error handling:
const handleDragStart = (event, row) => { if (!props.draggable) { + event.preventDefault() return } + try { // 去掉ghost image event.dataTransfer.setDragImage(new Image(), 0, 0) draggingState.dragged = row // 收起有子节点的节点 if (row.hasChildren) { setCollapse(row.id, true) } + } catch (error) { + console.warn('Failed to initialize drag operation:', error) + event.preventDefault() + } }
310-386: Consider reorganizing styles for better maintainability.The styles could be better organized by grouping related styles together and using CSS custom properties for repeated values.
Consider reorganizing the styles:
<style lang="less" scoped> +// Variables +:root { + --tree-row-height: 24px; + --tree-icon-size: 16px; + --tree-padding: 8px; + --tree-gap: 4px; +} + +// Layout .draggable-tree { .tree-row { - height: 24px; + height: var(--tree-row-height); width: fit-content; min-width: 100%; - padding: 0 8px; + padding: 0 var(--tree-padding); } // ... rest of the styles } +// Icons .expand-icon { - font-size: 16px; - width: 16px; - margin-right: 4px; + font-size: var(--tree-icon-size); + width: var(--tree-icon-size); + margin-right: var(--tree-gap); } +// Borders and visual states .border-top { // ... border styles }packages/plugins/tree/src/Main.vue (2)
167-195: Consider simplifying the handleDrop logic.The multiple early returns in the handleDrop function could be combined using a single condition to improve readability.
Apply this diff to simplify the logic:
const handleDrop = ({ dragged, target, position }) => { - // dragged和target相同，无需操作 - if (dragged.id === target.id) { - return - } - // 如果target节点为dragged节点的父节点，无需操作 - if (position === 'center' && target.rawData.children.some((item) => item.id === dragged.id)) { - return - } - // 如果相邻节点位置仍然不变，无需操作 - if (position !== 'center') { - const targetParentChildren = target.parent.rawData.children - const targetIndex = targetParentChildren.findIndex((item) => item.id === target.id) - const node = targetParentChildren[position === 'top' ? targetIndex - 1 : targetIndex + 1] - if (dragged.id === node?.id) { - return - } - } + const shouldSkip = + dragged.id === target.id || // Skip if dragged and target are the same + (position === 'center' && target.rawData.children.some((item) => item.id === dragged.id)) || // Skip if target is already the parent + (position !== 'center' && (() => { // Skip if position remains unchanged + const targetParentChildren = target.parent.rawData.children + const targetIndex = targetParentChildren.findIndex((item) => item.id === target.id) + const node = targetParentChildren[position === 'top' ? targetIndex - 1 : targetIndex + 1] + return dragged.id === node?.id + })()) + + if (shouldSkip) { + return + } const { insertNode, removeNode, selectNode } = useCanvas().canvasApi.value removeNode(dragged.id) insertNode( { data: dragged.rawData, node: target.rawData, parent: target.parent.rawData }, position === 'center' ? 'in' : position ) nextTick(() => { selectNode(dragged.id, 'clickTree') }) }
69-88: Consider optimizing the filterSchema function.The deep cloning of data using
extend(true, {}, data)could impact performance for large trees. Consider using a more efficient approach.Apply this diff to optimize the function:
const filterSchema = (data) => { const translateChild = (data) => { data.forEach((item) => { item.show = pageState.nodesStatus[item.id] !== false item.showEye = !item.show const child = item.children if (typeOf(child) !== 'array') { delete item.children } else { if (item.children.length) { translateChild(item.children) } } }) return data } - return [{ ...translateChild([extend(true, {}, data)])[0], componentName: 'body', id: 'body' }] + // Create a shallow copy of the data and only clone the necessary properties + const processedData = translateChild([{ ...data }])[0] + return [{ ...processedData, componentName: 'body', id: 'body' }] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/plugins/tree/src/DraggableTree.vue(1 hunks)packages/plugins/tree/src/Main.vue(6 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/plugins/tree/src/Main.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-engine#1050
File: packages/plugins/tree/src/Main.vue:151-165
Timestamp: 2025-02-06T08:58:58.022Z
Learning: The canvas API functions `getConfigure` and `allowInsert` in the Tiny Engine are internal APIs that don't require explicit error handling in the calling code.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (8)
packages/plugins/tree/src/Main.vue (6)
12-35: LGTM! The DraggableTree component is well integrated.The template structure is clean and the component is properly configured with all necessary props and event handlers.
41-47: LGTM! Clean imports and setup.The imports are well organized and the setup is properly structured.
151-195: LGTM! Well-structured drag and drop handlers.The drag and drop handlers are well implemented with:
- Clear validation logic in disallowDrop
- Comprehensive checks in handleDrop
- Helpful comments explaining the logic
233-263: LGTM! Clean and maintainable styles.The styles are well organized with proper use of CSS variables for theming.
12-35: LGTM! The DraggableTree integration looks good.The component is well-configured with proper props and event handlers. The slot content is structured effectively for rendering tree nodes.
233-263: LGTM! The styles are well-structured.The CSS classes are properly organized and make good use of CSS variables for theming.
packages/plugins/tree/src/DraggableTree.vue (2)
54-82: LGTM! Well-defined props and types.The props have proper validation and the type definitions are comprehensive.
Also applies to: 84-90
219-307: LGTM! Robust drag and drop implementation.The drag and drop handlers are well implemented with:
- Clear logic structure
- Proper edge case handling
- Helpful comments explaining complex logic
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/plugins/tree/src/Main.vue (2)
47-47: Consider moving DraggableTree to common components.Moving the
DraggableTreecomponent to the common components library would improve reusability across the project. This would allow other features to benefit from the dragging functionality while maintaining a single source of truth.
167-184: Simplify drag and drop validation logic.The multiple early returns and Chinese comments could be refactored for better maintainability.
Consider this refactor:
- const handleDrop = ({ dragged, target, position }) => { - // dragged和target相同，无需操作 - if (dragged.id === target.id) { - return - } - // 如果target节点为dragged节点的父节点，无需操作 - if (position === 'center' && target.rawData.children.some((item) => item.id === dragged.id)) { - return - } - // 如果相邻节点位置仍然不变，无需操作 - if (position !== 'center') { - const targetParentChildren = target.parent.rawData.children - const targetIndex = targetParentChildren.findIndex((item) => item.id === target.id) - const node = targetParentChildren[position === 'top' ? targetIndex - 1 : targetIndex + 1] - if (dragged.id === node?.id) { - return - } - } + const handleDrop = ({ dragged, target, position }) => { + const isInvalidDrop = + // Skip if dragging node onto itself + dragged.id === target.id || + // Skip if target is already the parent + (position === 'center' && target.rawData.children.some((item) => item.id === dragged.id)) || + // Skip if position remains unchanged + (position !== 'center' && isPositionUnchanged(dragged, target, position)) + + if (isInvalidDrop) { + return + }Add this helper function:
const isPositionUnchanged = (dragged, target, position) => { const targetParentChildren = target.parent.rawData.children const targetIndex = targetParentChildren.findIndex((item) => item.id === target.id) const node = targetParentChildren[position === 'top' ? targetIndex - 1 : targetIndex + 1] return dragged.id === node?.id }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/tree/src/Main.vue(6 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/plugins/tree/src/Main.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-engine#1050
File: packages/plugins/tree/src/Main.vue:151-165
Timestamp: 2025-02-06T08:58:58.022Z
Learning: The canvas API functions `getConfigure` and `allowInsert` in the Tiny Engine are internal APIs that don't require explicit error handling in the calling code.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/plugins/tree/src/Main.vue (2)
12-35: LGTM! Clean implementation of the draggable tree template.The template structure is well-organized with proper event bindings and visual feedback through icons.
233-263: LGTM! Well-structured styles with proper use of CSS variables.The styles are well-organized with proper use of CSS variables for theming and flexbox for layout.
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?
大纲树支持拖拽
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Refactor