refactor(styles): rewrite block styles #868
Conversation
refactor(styles): rewrite some block styles
…to refactor/block-styles
…ne into refactor/block-styles
fix/new ui
WalkthroughThe changes in this pull request involve multiple Vue components, enhancing their functionality, layout, and styling. Key modifications include the introduction of new props, restructuring templates, and improving user interactions through updated event handling. Notable components affected are Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (25)
packages/common/component/BlockDescription.vue (1)
2-7: Consider internationalizing the text contentThe Chinese text "设置区块暴露属性" should be moved to a translation file for better internationalization support.
- <span>设置区块暴露属性</span> + <span>{{ $t('block.setExposedProperties') }}</span>packages/plugins/block/src/BlockGroupArrange.vue (2)
18-18: Remove empty components option.The empty components object is unnecessary and can be removed to improve code cleanliness.
- components: {},
Line range hint
17-35: Consider migrating to TypeScript.To improve type safety and developer experience, consider converting this component to TypeScript. This would allow for better type definitions for props and state.
Example TypeScript interface for the arrangeList items:
interface ArrangeItem { id: string; name: string; svgName: string; } // Props interface interface Props { modelValue: string; arrangeList: ArrangeItem[]; }packages/plugins/materials/src/meta/block/src/BlockGroupArrange.vue (2)
68-70: LGTM with a minor suggestion for consistency.The flex layout and explicit dimensions improve the visual consistency of the component.
Consider consolidating the dimensions to CSS variables for better maintainability:
:deep(.tiny-tabs.tiny-tabs .tiny-tabs__item) { - width: 24px; - height: 24px; + width: var(--ti-lowcode-tab-item-size, 24px); + height: var(--ti-lowcode-tab-item-size, 24px); }Also applies to: 94-98
Line range hint
33-36: Consider extracting arrangement types to constants.The default value 'grid' and the type checking in
typeClicksuggest these are predefined arrangements. Consider extracting these to a constants file for better maintainability and reusability.Example implementation:
// constants.js export const BLOCK_ARRANGEMENTS = { GRID: 'grid', LIST: 'list' }; // In component import { BLOCK_ARRANGEMENTS } from './constants'; // Props modelValue: { type: String, default: BLOCK_ARRANGEMENTS.GRID, validator: (value) => Object.values(BLOCK_ARRANGEMENTS).includes(value) }packages/common/component/PluginBlockItemImg.vue (3)
6-12: Add accessibility attributes to improve screen reader support.The image element should include appropriate accessibility attributes:
<img v-if="item.screenshot" class="item-image" :src="item.screenshot || defaultImg" + :alt="item.name || 'Block preview image'" + role="img" draggable="false" @error="$event.target.src = defaultImg" />
40-44: Translate Chinese comment to English for consistency.- // 是否显示多选框 + // Whether to display checkbox
76-76: Document CSS custom properties.The component uses CSS custom properties for theming. Consider adding documentation for these variables:
// Add to the top of the style section: /** * CSS Custom Properties used: * --ti-lowcode-component-block-list-item-active-bg: Background color for active block items * --te-common-bg-default: Default background color for icons */Also applies to: 99-99
packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue (2)
Line range hint
97-116: Improve error handling and code structure.Several improvements could be made to this section:
- Error handling for the fetch call is missing
- The nested conditions could be simplified
- The error message is hardcoded and not internationalized
const checkBlock = (block) => { - if (selectedBlockArray.value.some((item) => item.id === block.id)) { - cancelCheck(block) - return - } + const isSelected = selectedBlockArray.value.some((item) => item.id === block.id); + if (isSelected) { + return cancelCheck(block); + } // 添加区块时，默认添加区块最新版本 if (!block.latestVersion) { - fetchBlockById(block.id).then((blockDetail) => { - const historyLength = blockDetail?.histories?.length || 0 - if (historyLength <= 0) { - const { message } = useModal() - message({ message: `${blockDetail.label}区块缺少历史记录，请重新发布区块`, status: 'error' }) - } else { - block.latestVersion = blockDetail?.histories[historyLength - 1]?.version - check(block) - } - }) + try { + const blockDetail = await fetchBlockById(block.id); + const histories = blockDetail?.histories || []; + + if (histories.length === 0) { + const { message } = useModal(); + message({ + message: t('block.error.no_history', { label: blockDetail.label }), + status: 'error' + }); + return; + } + + block.latestVersion = histories[histories.length - 1].version; + check(block); + } catch (error) { + const { message } = useModal(); + message({ + message: t('block.error.fetch_failed'), + status: 'error' + }); + } } else { check(block) } }
158-159: Consider using CSS custom properties for magic numbers.The height calculation uses a magic number (232px). Consider extracting this to a CSS custom property for better maintainability.
.block-add-transfer { flex: 1; display: flex; flex-direction: column; justify-content: space-between; - height: calc(100% - 232px); + height: calc(100% - var(--block-transfer-offset, 232px)); }packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (2)
4-4: Verify if the commented button code should be removed.The commented-out button code should be removed if it's no longer needed, rather than leaving it as a comment.
- <!-- <tiny-button type="primary" @click="selectVersion">确定</tiny-button> -->
Line range hint
75-117: Consider refactoring the version change logic for better maintainability.While the implementation is functional, the nested promise chain could be simplified for better readability and maintenance.
Consider restructuring like this:
- const handleChangeVersion = (selectedRow) => { - if (selectedRow) { - confirm({ - title: '修改区块版本', - message: '您确定要修改区块版本吗？', - exec: () => { - const params = { - groupId: selectedBlock.value.groupId, - blockId: selectedRow.block_id, - blockVersion: selectedRow.version - } - - requestGroupBlockVersion(params) - .then(() => { - isRefresh.value = true - closePanel() - confirm({ - title: '切换区块版本成功', - message: `${selectedBlock.value.label}区块，已切换为${selectedRow.version}版本，修改版本后需要刷新页面才生效，是否刷新页面？`, - exec: () => { - window.location.reload() - } - }) - }) - .catch((error) => { - message({ - message: `${selectedBlock.value.label}区块切换版本失败：${error.message || error}`, - status: 'error' - }) - }) - } - }) - } - } + const handleVersionChange = async (params) => { + try { + await requestGroupBlockVersion(params) + isRefresh.value = true + closePanel() + return true + } catch (error) { + message({ + message: `${selectedBlock.value.label}区块切换版本失败：${error.message || error}`, + status: 'error' + }) + return false + } + } + + const handleChangeVersion = async (selectedRow) => { + if (!selectedRow) return + + const confirmed = await confirm({ + title: '修改区块版本', + message: '您确定要修改区块版本吗？' + }) + + if (!confirmed) return + + const success = await handleVersionChange({ + groupId: selectedBlock.value.groupId, + blockId: selectedRow.block_id, + blockVersion: selectedRow.version + }) + + if (success) { + const shouldRefresh = await confirm({ + title: '切换区块版本成功', + message: `${selectedBlock.value.label}区块，已切换为${selectedRow.version}版本，修改版本后需要刷新页面才生效，是否刷新页面？` + }) + + if (shouldRefresh) { + window.location.reload() + } + } + }packages/plugins/materials/src/meta/block/src/BlockPanel.vue (1)
189-192: Consider using a CSS variable for padding valueWhile the styling changes look good, consider using a CSS variable for the padding value to maintain consistency across the application. This would make it easier to update spacing globally.
.block-list { - padding: 12px; + padding: var(--ti-lowcode-block-list-padding, 12px); overflow-y: auto; }packages/common/component/BindI18n.vue (4)
175-175: Style changes look good, with room for improvement.The spacing and alignment changes improve the layout consistency. Consider these optional enhancements:
- Use CSS variables for consistent spacing:
.languageContent { - margin-bottom: 16px; + margin-bottom: var(--ti-spacing-16); } .addNewLanguage { .tiny-input { - padding: 0 8px; + padding: 0 var(--ti-spacing-8); } }
- Make flex properties more explicit:
.tiny-input__inner { - flex: 1; + flex: 1 1 auto; }Also applies to: 200-205, 209-211, 214-214
Line range hint
8-13: Consider optimizing the filter method implementation.The current filterMethod implementation could be more efficient by:
- Debouncing the filter operation
- Using case-insensitive comparison
<tiny-select ref="selectRef" v-model="selectValue" placeholder="请根据显示值搜索" filterable :filter-method="filterMethod" @change="selectI18n" >Add debouncing in the script section:
import { debounce } from 'lodash-es' // In setup(): const filterMethod = debounce((value) => { const options = selectRef.value.state.cachedOptions const searchValue = value.toLowerCase() options.forEach((item) => { item.state.visible = value ? item.label.toLowerCase().includes(searchValue) : true }) }, 300)
Line range hint
92-108: Add type validation for parameters.The paramsChange method should validate parameter values before emitting.
const paramsChange = () => { const params = {} + // Validate parameter values + const isValid = paramsForm.value.every(({ name, value }) => { + if (!name || typeof value === 'undefined') { + console.warn(`Invalid parameter: ${name}`) + return false + } + return true + }) + if (!isValid) return paramsForm.value.forEach(({ name, value }) => { params[name] = value }) emit('bind', { type: PROP_DATA_TYPE.I18N, key: selectValue.value, params }) }
Line range hint
110-116: Enhance editForm initialization.The openCreateForm method could be more robust with proper type initialization.
const openCreateForm = () => { - Object.keys(editForm).forEach((key) => delete editForm[key]) + // Reset form with default values + const defaultForm = { + key: `lowcode.${utils.guid()}`, + type: PROP_DATA_TYPE.I18N + } + + // Initialize language fields + props.locales.forEach(({ lang }) => { + defaultForm[lang] = '' + }) + + Object.assign(editForm, defaultForm) - editForm.key = 'lowcode.' + utils.guid() - editForm.type = PROP_DATA_TYPE.I18N showEditItem.value = true }packages/plugins/materials/src/meta/block/src/BlockList.vue (1)
52-65: Standardize props documentation styleThe props documentation style is inconsistent:
showBlockShotuses block commentsshowCheckboxandcheckeduse inline commentsgridColumnslacks documentationConsider using block comments consistently for all props to improve maintainability.
showBlockShot: { type: Boolean, default: true }, - // 是否显示多选框 + /* + 是否显示多选框 + */ showCheckbox: { type: Boolean, default: false }, - // 选中的区块 + /* + 选中的区块 + */ checked: { type: Array, default: () => [] }, + /* + 网格列数 + */ gridColumns: { type: Number, default: 2 }packages/common/component/MetaListItem.vue (1)
39-46: Consider extracting modal logic into a composable.The component handles multiple state variables (
isShow,isVisible,showMask) for modal/popover management. Consider extracting this logic into a reusable composable function (e.g.,useModal) to improve maintainability and reusability across similar components.Example structure:
// useModal.ts export function useModal() { const isVisible = ref(false) const showMask = ref(false) const show = () => { isVisible.value = true showMask.value = true } const hide = () => { isVisible.value = false showMask.value = false } return { isVisible, showMask, show, hide } }packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
Line range hint
117-141: Improve error handling in addBlocks methodWhile the changes improve the method with early returns and success notifications, the error handling could be enhanced.
Consider returning early after showing the error message to prevent continuing with panel closure:
.catch((error) => { message({ message: `添加区块失败: ${error.message || error}`, status: 'error' }) + return }) panelState.isBlockGroupPanel = false closePanel()packages/plugins/page/src/PageTree.vue (2)
268-282: LGTM with a minor suggestion on class namesThe refactored template structure is cleaner and more maintainable. However, consider using more semantic class names:
- <SvgIcon name="text-page-common" class="icon-page"></SvgIcon> + <SvgIcon name="text-page-common" class="page-icon"></SvgIcon>
Line range hint
364-420: LGTM with style optimization suggestionsThe style changes improve maintainability and visual consistency. Consider these optimizations:
.app-manage-tree { :deep(.tiny-tree) { .tiny-tree-node__label { font-size: 12px; display: flex; align-items: center; flex: 1; - & > .svg-icon { + .svg-icon { margin-right: 4px; } - & .label { + .label { font-size: 12px; flex: 1; } } .tree-node-icon { margin-right: 0; } - .svg-icon { - width: 14px; - height: 14px; - } + .svg-icon { + width: 14px; + height: 14px; + } .icons { display: flex; align-items: center; gap: 4px; } .tiny-tree-node.is-leaf .tiny-tree-node__content { padding-left: 0; } .tiny-tree-node.is-current { - & > .tiny-tree-node__content { - & > div > .tiny-tree-node__content-box { - background-color: var(--te-common-bg-container); - } + .tiny-tree-node__content { + .tiny-tree-node__content-box { + background-color: var(--te-common-bg-container); + } } } } }These changes:
- Remove unnecessary nesting with
&- Consolidate identical selectors
- Simplify the selector hierarchy
packages/plugins/block/src/composable/useBlock.js (1)
689-699: Well-structured refactoring of block selection logic.The changes to both functions represent a good architectural improvement:
- Simplified API by removing the unnecessary
blockListparameter- Consistent use of immutable array operations
- Proper integration with Vue's reactivity system
Consider documenting these functions with JSDoc comments to clarify:
- Parameter types and requirements
- Return values
- Usage examples
packages/common/component/PluginBlockList.vue (2)
434-440: Update deprecated:deep()to::v-deepin scoped CSSThe
:deep()combinator is deprecated in Vue 3 in favor of::v-deep. Updating to the new syntax ensures compatibility with future versions of Vue.Apply this diff:
-.col-checkbox, -.block-item-small-list:deep(.table-selection) { +.col-checkbox, +.block-item-small-list::v-deep(.table-selection) { width: 40px; text-align: center; } -.col-checkbox:deep(.tiny-checkbox__label) { +.col-checkbox::v-deep(.tiny-checkbox__label) { padding: 0; }
489-493: Use::v-deepinstead of:deep()for deep selectorsConsistently update all instances of
:deep()to::v-deepin your scoped CSS to align with Vue 3 conventions.Apply this diff:
- &:deep(.block-item-img) { + &::v-deep(.block-item-img) { width: 54px; height: 40px; flex: unset; margin-left: 8px; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
packages/design-core/assets/block-add-prop.svgis excluded by!**/*.svgpackages/design-core/assets/block-bind-prop.svgis excluded by!**/*.svgpackages/design-core/assets/text-copy-page.svgis excluded by!**/*.svgpackages/design-core/assets/编辑2.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
- packages/common/component/BindI18n.vue (2 hunks)
- packages/common/component/BlockDescription.vue (2 hunks)
- packages/common/component/BlockLinkField.vue (3 hunks)
- packages/common/component/ConfigItem.vue (1 hunks)
- packages/common/component/MetaListItem.vue (1 hunks)
- packages/common/component/PluginBlockItemImg.vue (1 hunks)
- packages/common/component/PluginBlockList.vue (14 hunks)
- packages/plugins/block/src/BlockGroupArrange.vue (1 hunks)
- packages/plugins/block/src/Main.vue (2 hunks)
- packages/plugins/block/src/composable/useBlock.js (1 hunks)
- packages/plugins/materials/src/meta/block/src/BlockGroupArrange.vue (4 hunks)
- packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (6 hunks)
- packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue (6 hunks)
- packages/plugins/materials/src/meta/block/src/BlockList.vue (2 hunks)
- packages/plugins/materials/src/meta/block/src/BlockPanel.vue (2 hunks)
- packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (5 hunks)
- packages/plugins/page/src/PageTree.vue (6 hunks)
- packages/settings/props/src/Main.vue (1 hunks)
🧰 Additional context used
🔇 Additional comments (39)
packages/settings/props/src/Main.vue (3)
Line range hint
9-44: LGTM! Well-structured reactive logicThe script implementation follows Vue Composition API best practices with proper:
- Reactive state management using ref and computed
- Side effect handling with watchEffect
- Clean component registration
45-47: Verify margin consistency with design systemThe margin change from 10px to 12px should align with the design system's spacing scale.
Let's check for consistency in spacing values:
#!/bin/bash # Description: Check for consistent margin usage across components # Look for margin declarations in Vue files rg "margin:\s+\d+px" --type vue
Line range hint
1-8: Verify Empty component implementation and user experienceThe Empty component's visibility is controlled by
showEmptyTips, but we should ensure this provides a good user experience when properties are empty.Let's check the Empty component implementation:
✅ Verification successful
Let me try different search strategies to verify the Empty component implementation.
The previous command failed due to file type issues, so I'm using more generic searches to find the relevant files. This will help us verify:
- Where the Empty component is defined
- How showEmptyTips is used across the codebase
- Other instances of Empty component usage for consistency
Let me check the Empty component implementation to complete the verification.
This will help us verify:
- The implementation of the Empty component being used
- Whether it follows consistent patterns with other empty state components
- The proper handling of the showEmptyTips prop
Empty component implementation is properly handled and follows best practices
The Empty component implementation is well-designed and provides appropriate feedback:
- It correctly uses the
showEmptyTipsprop to control visibility- It dynamically updates the message based on canvas state (empty canvas vs no selection)
- It integrates with the broadcast channel system to react to schema changes
- The styling is consistent with the design system using CSS variables
The implementation provides good user experience by:
- Showing different messages for different empty states
- Using appropriate styling for empty state messaging
- Reactively updating based on user interactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Empty component implementation and usage patterns # Look for other usages of Empty component and its showEmptyTips prop rg -l "showEmptyTips" --type vueLength of output: 162
Script:
#!/bin/bash # Search for Empty component definition and usage # First, find files containing "Empty" component references rg -l "Empty" # Then search for showEmptyTips prop usage rg "showEmptyTips" # Also look for Empty component definition fd -e vue -e js -e ts EmptyLength of output: 2744
Script:
#!/bin/bash # Check the Empty component implementation cat packages/settings/props/src/components/Empty.vue # Also check if there's any shared empty component implementation cat packages/common/component/EmptyTip.vueLength of output: 2158
packages/common/component/BlockDescription.vue (1)
39-60: Well-structured styling improvementsThe style changes demonstrate good practices:
- Proper use of design system variables
- Modern layout techniques with flexbox
- Consistent spacing and sizing
- Clean nesting structure with Less
packages/plugins/materials/src/meta/block/src/BlockGroupArrange.vue (2)
6-8: Verify the UX impact of removing tooltips.While removing the tooltip wrapper simplifies the code, consider whether this impacts user experience, especially for new users who might need guidance on what each icon represents.
Consider adding
titleattributes to maintain basic hover tooltips:- <span class="button-icon-item" @click="typeClick(item.id)"> + <span class="button-icon-item" @click="typeClick(item.id)" :title="item.id">
17-17: LGTM!The import statement is correctly updated to reflect the removal of the tooltip component.
packages/common/component/PluginBlockItemImg.vue (2)
51-63: Clean implementation of event handling.The setup function follows Vue 3 Composition API best practices with proper computed property usage and event emission.
27-30: Verify component integration with PluginBlockList.The component appears to be part of a larger refactoring effort. Let's verify its integration:
✅ Verification successful
Let me try a different approach to verify the component integration.
This should help us understand:
- Where PluginBlockItemImg is being used
- Where PluginBlockList exists and its relationship
- The usage pattern of Checkbox component
- The overall Vue component structure
Let me check the integration details between these components.
This will help us understand:
- How PluginBlockItemImg is integrated within PluginBlockList
- The relationship with BlockList.vue in materials plugin
- How these components are exported and made available to other parts of the application
Based on the gathered information, I can now provide a final response:
Component integration is properly structured and verified.
The verification shows that:
PluginBlockItemImgis correctly imported and registered inPluginBlockList.vue- The component is properly integrated within the common components ecosystem, with
PluginBlockListbeing exported in theindex.js- The Checkbox import from
@opentiny/vueis appropriate as there are no conflicting checkbox implementations in the common components🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of PluginBlockItemImg component rg -l "PluginBlockItemImg" --type vue # Check for any existing checkbox-related components that might conflict ast-grep --pattern 'import { Checkbox } from "@opentiny/vue"'Length of output: 129
Script:
#!/bin/bash # Search for any file containing PluginBlockItemImg, without file type restriction rg -l "PluginBlockItemImg" # Search for files containing PluginBlockList to understand integration rg -l "PluginBlockList" # Look for Checkbox imports or usage patterns rg -l "Checkbox" # Find all Vue files in the repository fd ".*\.vue$"Length of output: 18802
Script:
#!/bin/bash # Check the content of PluginBlockList.vue to understand how it uses PluginBlockItemImg rg -A 10 "PluginBlockItemImg" "packages/common/component/PluginBlockList.vue" # Check how BlockList.vue in materials plugin uses these components rg -A 10 "PluginBlockItemImg" "packages/plugins/materials/src/meta/block/src/BlockList.vue" # Check the exports in common/component/index.js cat "packages/common/component/index.js"Length of output: 5446
packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue (3)
29-29: LGTM!The import and registration of the BlockList component is clean and follows Vue best practices.
Also applies to: 36-36
141-142: LGTM!The state management and exports are clean and well-structured.
11-21: Verify that this refactor maintains existing functionality.The replacement of
BlockGroupTransferPanelwithBlockListintroduces new behavioral props (trigger-check,show-block-shot, etc.) which suggest functional changes beyond the stated PR objective of style refactoring.packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (5)
8-26: LGTM! Improved grid structure with better user interaction.The grid modifications improve the UX by:
- Providing clear column headers
- Adding direct per-row actions
- Showing formatted dates
38-39: LGTM! Clean import organization.The imports are properly organized and aligned with the component's needs.
118-120: LGTM! Clear and concise version check.The
isCurrentVersionmethod is well-implemented and serves its purpose effectively.
127-130: LGTM! Clean method exports.The exports are well-organized and include all necessary methods.
145-153: LGTM! Well-structured button styles.The styles are properly scoped and use appropriate CSS variables for theming.
packages/plugins/materials/src/meta/block/src/BlockPanel.vue (1)
7-9: LGTM: Clean structural improvementThe addition of the
.block-listwrapper div improves the layout organization while maintaining the existing functionality. This change aligns well with the PR's style refactoring objective.packages/common/component/BlockLinkField.vue (4)
15-22: LGTM: Improved menu item clarity.The addition of icons and more descriptive text enhances the user experience by making actions more intuitive.
67-67: LGTM: Simplified dialog title.The dialog title change makes it more concise while maintaining clarity.
2-2: Verify the absolute positioning of icons.The template now uses absolutely positioned icons with z-index management. While this creates an interesting visual effect, it might cause layout issues on different screen sizes or when the parent container has specific positioning contexts.
#!/bin/bash # Search for other components using similar icon positioning to ensure consistency rg -l "transform:\s*translate\(-50%,\s*-50%\)" --type cssAlso applies to: 4-11
Line range hint
217-221: Investigate and document the max-height requirement.The comment indicates uncertainty about the max-height property. This should be investigated and either properly documented or removed if unnecessary.
✅ Verification successful
Let me try different search patterns to gather more information about this popover styling.
This should help us understand:
- The context where this popover class is used
- Similar max-height patterns across the codebase
- Related popover styling patterns
- Which Vue components actually use this popover
Based on the search results, I can now provide a final response about the max-height property in the block-new-attr-popover class.
The max-height property is necessary and should be documented.
The max-height: 65vh property is used consistently across several popover implementations in the codebase, particularly for block-related popovers. This is a deliberate design choice to:
- Prevent the popover from extending beyond the viewport height
- Maintain consistency with other similar popovers in the application
- Ensure content remains scrollable when it exceeds the viewport height
The comment questioning its necessity can be replaced with proper documentation explaining its purpose:
.tiny-popover.tiny-popper.tiny-popper.block-new-attr-popover[x-placement] { /* Limits popover height to 65% of viewport height to ensure content remains accessible and scrollable */ max-height: 65vh; padding: 0; }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of max-height in popovers rg "max-height.*vh" --type css # Search for the block-new-attr-popover class usage rg "block-new-attr-popover" --type vueLength of output: 196
Script:
#!/bin/bash # Search for the specific class name with broader context rg "block-new-attr-popover" -A 5 -B 5 # Search for popover-related max-height styles rg "max-height.*vh" # Search for tiny-popover class usage rg "tiny-popover" -A 5 -B 5 # Look for any Vue files that might use this popover fd -e vue -x grep -l "block-new-attr-popover" {}Length of output: 92991
packages/plugins/materials/src/meta/block/src/BlockList.vue (1)
10-12: LGTM: Props binding follows Vue conventionsThe new props are correctly bound to the plugin-block-list component following Vue's kebab-case naming convention.
packages/common/component/MetaListItem.vue (2)
39-39: LGTM: Clean event handling implementation.The direct click handler on the icon-edit component maintains functionality while reducing component nesting depth by removing the tooltip wrapper.
44-46: Consider adding type validation for dynamic components.While the dynamic component implementation is clean, consider adding validation to ensure
item.iconexists and is a valid component to prevent potential runtime errors.Let's check how these icons are used across the codebase:
Add type validation before rendering:
- <component :is="item.icon"></component> + <component v-if="item.icon" :is="item.icon"></component> + <span v-else class="icon-placeholder"></span>packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (4)
32-32: LGTM: Clean implementation of state managementThe addition of
blockUsersref and its provision to child components follows Vue 3's Composition API best practices for state management.Also applies to: 36-36, 77-78
234-234: LGTM: Proper state update in getFiltersThe assignment to
blockUsers.valuecorrectly updates the shared state after fetching user data.
267-267: LGTM: Improved responsive layoutReplacing fixed width with
flex: 1improves the component's responsiveness and flexibility.
Line range hint
32-267: Verify PR description accuracyThe PR is described as a style-only refactoring, but it includes several functional enhancements:
- Added new state management with
blockUsers- Enhanced error handling in
addBlocks- Added success notifications
- Improved early returns
Consider updating the PR description to accurately reflect these functional improvements.
packages/plugins/page/src/PageTree.vue (2)
28-28: LGTM: Clean icon handling refactorThe change simplifies the tree node icon handling by using a consistent empty icon approach, which aligns well with the refactoring goals.
285-285: LGTM: Consistent icon handling implementationThe icon-related changes are well-structured and follow Vue 3 composition API best practices.
Also applies to: 317-317, 331-331
packages/plugins/block/src/Main.vue (2)
84-95: LGTM: Improved layout structureThe addition of a wrapper div with proper class name improves the component's structure and enables better scroll behavior management through CSS.
481-482: LGTM: Enhanced layout stylingThe added padding and overflow properties improve the component's visual presentation and scrolling behavior.
packages/plugins/block/src/composable/useBlock.js (2)
698-699: Consider adding parameter validation.The function logic is correct and the simplified signature is an improvement. However, consider adding defensive checks for the block parameter.
const cancelCheck = (block) => { + if (!block?.id) { + return + } selectedBlockArray.value = selectedBlockArray.value.filter((item) => item.id !== block.id) }Let's verify all callers have been updated to the new signature:
#!/bin/bash # Search for calls to the cancelCheck function rg "cancelCheck\(.*?,.*?\)" --type vue
689-694: Consider adding parameter validation.The function logic is correct and the simplified signature is an improvement. However, consider adding defensive checks for the block parameter.
const check = (block) => { + if (!block?.id) { + return + } if (selectedBlockArray.value.some((item) => item.id === block.id)) { return } selectedBlockArray.value = selectedBlockArray.value.concat(block) }Let's verify all callers have been updated to the new signature:
✅ Verification successful
Let me try a different approach to verify the usage of the
checkfunction.The first command will show us the actual calls with context, while the second command will list files containing potential calls to the function. This will help us verify if all callers have been updated to use the new single-parameter signature.
Additionally, let's check the AST for function calls:
This broader search strategy should help us find all relevant usages of the
checkfunction.
Based on the search results, I can now provide a final response:
The signature change is safe and all callers are using the new single-parameter pattern
The search results show that all existing calls to the
checkfunction in the codebase are already using the single-parameter pattern:
- In
packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue:
check(block)check(block)The only other occurrence is an unrelated
check()method inCanvasMenu.vuethat serves a different purpose (checking container configuration).Therefore, my original review suggestion for adding parameter validation still stands, but there's no need for concern about breaking changes in the function signature as all callers are already aligned with the new pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to the check function rg "check\(.*?,.*?\)" --type vueLength of output: 68
Script:
#!/bin/bash # Search for calls to the check function in all files rg "check\(" -A 2 # Also search specifically in .js and .vue files find . -type f \( -name "*.js" -o -name "*.vue" \) -exec grep -l "check(" {} \;Length of output: 1057
Script:
#!/bin/bash # Search for check function calls in the AST ast-grep --pattern 'check($$$)'Length of output: 33
packages/common/component/ConfigItem.vue (1)
541-542: LGTM: Style changes improve error popup positioning.The addition of
position: relativeandoverflow: visibleto.item-inputensures proper positioning and visibility of error popups. These changes are necessary for the error feedback UI to work correctly.packages/common/component/PluginBlockList.vue (4)
28-33: Usage of<plugin-block-item-img>component is correctThe integration of
<plugin-block-item-img>with the specified props ensures proper rendering and functionality of block items, especially in 'mini' block style.
39-45: Display of creation time and creator is properly implementedThe code correctly displays the 'created_at' and 'createdBy' fields when
blockStyleis 'mini', enhancing the information presented to the user.
208-219: New props enhance component configurabilityThe addition of
showCheckbox,checked, andgridColumnsprops with appropriate default values adds flexibility to the component, allowing for better customization of its behavior and layout.
225-225: Injection of 'blockUsers' is appropriately handledThe injected
blockUsersis safely handled with optional chaining and defaults to an empty array if not provided, ensuring robustness.
packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (2)
Line range hint
74-119: Consider a less disruptive update approach.While the version change logic is well-implemented with proper error handling and user confirmations, forcing a page reload might disrupt the user experience. Consider implementing a more seamless update mechanism that only refreshes the affected components.
Example approach:
// Instead of window.location.reload() const refreshAffectedComponents = () => { isRefresh.value = true; // Emit an event to notify parent components emit('version-changed', { blockId: selectedBlock.value.id, version: selectedRow.version }); };
144-152: Consider simplifying the selector specificity.While the styles are well-organized, the double class selector
.tiny-button.tiny-buttonseems unnecessarily specific. This might make the styles harder to maintain or override if needed.Consider simplifying to:
-.tiny-button.tiny-button { +.tiny-button { &.fallback-btn { min-width: unset; padding: 0; } &.tiny-button--text.is-disabled { color: var(--ti-button-text-color-disabled); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/common/component/BlockDescription.vue (2 hunks)
- packages/common/component/PluginBlockItemImg.vue (1 hunks)
- packages/common/component/PluginBlockList.vue (14 hunks)
- packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/common/component/BlockDescription.vue
- packages/common/component/PluginBlockItemImg.vue
- packages/common/component/PluginBlockList.vue
🔇 Additional comments (3)
packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (3)
7-25: LGTM! Well-structured grid implementation.The grid template is well-organized with clear column definitions and proper conditional rendering for the fallback button.
37-38: LGTM! Good use of utility functions.Proper utilization of the date format utility from @opentiny/vue-renderless/common/date.
126-129: LGTM! Clean component exports.The exports are well-organized and properly follow Vue Composition API patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/plugins/page/src/PageFolderSetting.vue (4)
Line range hint
5-7: Add i18n support for button textThe "保存" (Save) text should be internationalized for better maintainability and multi-language support.
-<tiny-button type="primary" @click="saveFolderSetting">保存</tiny-button> +<tiny-button type="primary" @click="saveFolderSetting">{{ t('save') }}</tiny-button>
Line range hint
8-12: Enhance button accessibilityAdd appropriate ARIA labels to improve accessibility for screen readers.
<svg-button v-if="!pageSettingState.isNew" name="delete" placement="bottom" tips="删除" + aria-label="Delete folder" @click="deleteFolder" ></svg-button>
Line range hint
182-191: Fix incorrect notification type in delete error handlerThe error notification is using 'success' type instead of 'error'.
.catch((error) => { useNotify({ - type: 'success', + type: 'error', title: '删除文件夹失败！', message: JSON.stringify(error?.message || error) }) })
Line range hint
196-197: Consider reducing the throttle duration for delete operationThe current 5-second throttle might be frustrating for users. Consider reducing it to 1-2 seconds for better user experience while still preventing accidental double-clicks.
- deleteFolder: throttle(5000, true, deleteFolder), + deleteFolder: throttle(1000, true, deleteFolder),packages/plugins/page/src/PageTree.vue (1)
268-282: LGTM! Clean and semantic icon rendering logic.The conditional rendering is clear and well-structured, with appropriate icon names that reflect their purpose.
Consider moving the icon names to constants for better maintainability:
+const ICON_NAMES = { + PAGE: 'text-page-common', + FOLDER: 'text-page-folder-closed', + LOCKED: 'locked', + HOME: 'text-page-home', + SETTINGS: 'setting' +} renderContent = (h, { node, data }) => { return ( <span class="tiny-tree-node__label" onMousedown={(e) => nodeClick(e, node)}> {data.isPage ? ( - <SvgIcon name="text-page-common" class="icon-page"></SvgIcon> + <SvgIcon name={ICON_NAMES.PAGE} class="icon-page"></SvgIcon> ) : ( - <SvgIcon name="text-page-folder-closed" class="folder-icon"></SvgIcon> + <SvgIcon name={ICON_NAMES.FOLDER} class="folder-icon"></SvgIcon> )}packages/plugins/page/src/PageSetting.vue (1)
Line range hint
1-431: Consider splitting the component for better maintainability.The component currently handles multiple responsibilities. Consider breaking it down into smaller, more focused components:
PageSettingsForm: Handle form validation and data managementPageOperations: Manage page operations (create/copy/delete)PageHistory: Handle history-related functionalityThis would improve:
- Code maintainability
- Testing capabilities
- Reusability
- State management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/plugins/page/src/PageFolderSetting.vue (1 hunks)
- packages/plugins/page/src/PageSetting.vue (1 hunks)
- packages/plugins/page/src/PageTree.vue (6 hunks)
🔇 Additional comments (4)
packages/plugins/page/src/PageFolderSetting.vue (1)
211-229: LGTM: Style improvements enhance UI consistencyThe style changes effectively create a cleaner, more modern interface by:
- Removing redundant borders
- Implementing consistent button sizing and spacing
- Adjusting padding for better content layout
packages/plugins/page/src/PageTree.vue (1)
381-424: LGTM! Well-structured and maintainable styles.The styling updates demonstrate good practices:
- Use of CSS variables for theming
- Modern layout techniques (flex, gap)
- Clear visual hierarchy
- Proper scoping with :deep() selector
packages/plugins/page/src/PageSetting.vue (2)
428-431: LGTM: Style enhancement improves visual consistency.The removal of the bottom border from the collapse component is a valid styling improvement that aligns with the PR's refactoring objectives.
428-431: 🛠️ Refactor suggestionConsider enhancing error handling and user feedback.
While the implementation of page operations is generally good, consider these improvements:
- Add more specific error messages instead of using
JSON.stringify(err?.message || err).- Consider adding loading states during async operations.
- Add retry mechanism for network failures.
Example improvement for error handling:
requestCreatePage(createParams) .then((data) => { pageSettingState.updateTreeData() // ... rest of success handling }) .catch((err) => { - useNotify({ - type: 'error', - title: '新建页面失败', - message: JSON.stringify(err?.message || err) - }) + const errorMessage = err?.response?.data?.message || err?.message || '未知错误' + useNotify({ + type: 'error', + title: '新建页面失败', + message: `创建页面时发生错误: ${errorMessage}` + }) })Likely invalid or redundant comment.
…to refactor/block-styles
已修改 |
Modified |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/common/component/LifeCycles.vue (2)
Line range hint
2-4: Consider improving accessibility and UX of the popover.The popover's hover-only trigger (
trigger="hover") may cause accessibility issues for keyboard and screen reader users. Consider:
- Adding
clicktrigger support- Including keyboard navigation
- <tiny-popover v-model="state.showPopover" placement="bottom-end" trigger="hover" popperClass="option-popper"> + <tiny-popover v-model="state.showPopover" placement="bottom-end" trigger="click" popper-class="option-popper">
Line range hint
156-164: Add error handling to syncLifeCycle function.The function directly accesses and mutates the schema without proper error handling. Consider adding try-catch and null checks.
const syncLifeCycle = () => { + try { const currentSchema = useCanvas().canvasApi.value?.getSchema?.() + if (!currentSchema) { + console.warn('Schema not available') + return + } const pageContent = getPageContent() const { id, fileName } = pageContent if (id === currentSchema.id || fileName === currentSchema.fileName) { currentSchema.lifeCycles = state.bindLifeCycles } + } catch (error) { + console.error('Failed to sync lifecycle:', error) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/common/component/LifeCycles.vue (1 hunks)
🔇 Additional comments (2)
packages/common/component/LifeCycles.vue (2)
275-279: LGTM! Style changes improve layout consistency.The margin and padding adjustments enhance the visual hierarchy while maintaining the theme system through CSS variables.
Line range hint
1-391: Verify: Changes align with PR objectives.The modifications focus on style refactoring without introducing functional changes, which aligns perfectly with the PR's stated objectives. The component's behavior and API remain unchanged while the visual presentation is improved.
✅ Verification successful
Let me analyze the changes in the LifeCycles.vue component by comparing its props and emits with the search results.
Based on the git history and the current code review, I can now generate the final response:
Style changes verified, no API modifications detected
The changes in LifeCycles.vue are purely cosmetic, with no modifications to the component's API contract:
- Props remain unchanged:
bindLifeCycles(Object) andisPage(Boolean)- Emits are preserved:
updatePageLifeCyclesandbind- All functional behavior is maintained while only styling aspects were updated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no API changes were introduced # Search for any changes to props or emits rg -p "props:|emits:" .Length of output: 23831
Script:
#!/bin/bash # Get git history for the file to see if there were any changes to props or emits git log -p packages/common/component/LifeCycles.vue | grep -A 5 "^+.*props:\|^-.*props:\|^+.*emits:\|^-.*emits:"Length of output: 464
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
Release Notes
New Features
PluginBlockItemImg) with optional checkbox functionality.PluginBlockItemImgcomponent.Improvements
Bug Fixes
Styling Updates