fix: fix grid、flowchart、grid、tree-menu bugs #2588
Conversation
WalkthroughThis pull request includes several modifications across various files, primarily focusing on dependency management, component structure, and styling adjustments. Key changes involve the removal of specific dependencies, updates to import paths, and alterations to CSS properties for better component responsiveness. Notably, the Changes
Possibly related PRs
Suggested labels
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![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
🧹 Outside diff range and nitpick comments (5)
packages/vue/src/tree-menu/src/props.ts (1)
Line range hint
89-93: Improve documentation and naming for the new prop.
- The comment "metaErp" reveals internal implementation details and should be removed or clarified.
- The
widthAdaptprop lacks documentation about its purpose and usage.- Consider a more descriptive name like
adaptiveWidthorautoWidth.- // metaErp 新增属性 - widthAdapt: { + /** + * Enable automatic width adaptation for the tree menu + * @default false + */ + adaptiveWidth: { type: Boolean, default: false }packages/vue/src/grid/src/radio/src/radio.ts (1)
53-63: Consider adding error handling for missing slotsThe labelContent function handles Vue version compatibility well, but it might throw an error if slots.default is undefined.
Consider adding a null check:
const labelContent = () => { let content = h( 'span', { class: 'tiny-grid-radio__label', directives: isVue2 ? [{ name: 'AutoTip' }] : null }, - $slots.default() + $slots.default?.() || '' ) if (!isVue2) { hooks.withDirectives(content, [[AutoTip]]) } return content }packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts (1)
79-85: Consider improving readability and performanceWhile the logic is correct, the condition checking could be more readable and performant.
Consider extracting the conditions and caching DOM queries:
- const tableContent = _vm.$refs.tableBody?.$refs.table + const { tableHeader, tableBody } = _vm.$refs + const tableContent = tableBody?.$refs.table + const headerEl = tableHeader?.$el + + const isOutsideTable = !_vm.getEventTargetNode(event, $el).flag + const isInHeader = headerEl?.contains(event.target) + const isOutsideContent = tableContent && !tableContent.contains(event.target) + if ( isClear || - !_vm.getEventTargetNode(event, $el).flag || - (_vm.$refs.tableHeader && _vm.$refs.tableHeader.$el.contains(event.target)) || - (tableContent && !tableContent.contains(event.target)) + isOutsideTable || + isInHeader || + isOutsideContent )packages/vue/src/tree-menu/src/pc.vue (1)
135-135: Document the purpose of mono parameter.The addition of
mono: truein the setup function needs documentation to explain its purpose and impact.Add a comment explaining the purpose of the mono parameter:
- setup(props, context) { - return setup({ props, context, renderless, api, mono: true }) + // mono: true enables single-instance mode for renderless logic + setup(props, context) { + return setup({ props, context, renderless, api, mono: true })Also applies to: 137-137
packages/theme/src/old-theme-index.js (1)
5-5: LGTM: New theme variables enhance flexibility.The addition of new CSS variables (
--tv-border-radius-brand,--tv-space-xxl,--tv-space-xxxl) enhances theme customization options.Consider adding comments to document the intended use cases for these new variables in the theme documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/theme/src/svgs/ellipsis.svgis excluded by!**/*.svg
📒 Files selected for processing (17)
examples/sites/package.json(1 hunks)packages/modules.json(0 hunks)packages/renderless/src/flowchart/index.ts(1 hunks)packages/renderless/types/tree-menu.type.ts(1 hunks)packages/theme-saas/src/cascader-menu/index.less(1 hunks)packages/theme-saas/src/date-range/index.less(1 hunks)packages/theme-saas/src/grid/mixins/table.less(1 hunks)packages/theme-saas/src/grid/table.less(0 hunks)packages/theme-saas/src/picker/index.less(2 hunks)packages/theme/src/old-theme-index.js(1 hunks)packages/vue/src/file-upload/src/mobile-first.vue(1 hunks)packages/vue/src/grid/package.json(1 hunks)packages/vue/src/grid/src/radio/src/radio.ts(4 hunks)packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts(1 hunks)packages/vue/src/tree-menu/index.ts(1 hunks)packages/vue/src/tree-menu/src/pc.vue(2 hunks)packages/vue/src/tree-menu/src/props.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/modules.json
- packages/theme-saas/src/grid/table.less
✅ Files skipped from review due to trivial changes (1)
- packages/renderless/types/tree-menu.type.ts
🔇 Additional comments (13)
packages/vue/src/tree-menu/index.ts (1)
12-12: Verify the impact of import path change
The import path change from './src/index' to './src/pc.vue' might affect existing imports and component registration.
✅ Verification successful
Based on the comprehensive search results, I can now generate the final response:
Import path change is safe and intentional
The change from './src/index' to './src/pc.vue' is part of a valid structural reorganization, as evidenced by:
- The component is properly registered and exported in the new location (pc.vue)
- No other files in the codebase are importing from the old path './src/index'
- The component's functionality is maintained in pc.vue with all necessary features
- The change aligns with the component's registration in modules.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other files still importing from the old path
rg -l "from.*['\"].*tree-menu/src/index['\"]"
# Check if there are any direct imports of TreeMenuPc
rg -l "TreeMenuPc"
Length of output: 70
Script:
#!/bin/bash
# Let's try a broader search to understand the tree-menu usage
rg -l "tree-menu"
# Check the content of the old index file if it exists
fd "index.(ts|js|vue)" packages/vue/src/tree-menu -x cat {}
# Check the content of pc.vue
fd "pc.vue" packages/vue/src/tree-menu -x cat {}
# Look for any TreeMenu imports or usage patterns
rg "TreeMenu" -A 2
Length of output: 64204
packages/theme-saas/src/cascader-menu/index.less (1)
21-21: LGTM! Good improvement for UI flexibility
Changing from fixed height to min-height is a good improvement that prevents content truncation while maintaining the minimum size requirement.
packages/vue/src/grid/package.json (1)
19-19: LGTM! Verify dependency consistency.
The addition of @opentiny/vue-directive as a workspace dependency is appropriate for the AutoTip directive integration.
✅ Verification successful
Let me verify the actual dependency configuration in these files to ensure consistent versioning.
Dependency configuration is consistent across all packages
The @opentiny/vue-directive dependency is consistently configured with workspace:~ version specifier across all packages that use it, including the grid package. This maintains uniform version management throughout the workspace.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent workspace protocol usage across related packages
rg -l "\"@opentiny/vue-directive\":" | xargs grep -l "workspace:" || echo "No inconsistencies found"
Length of output: 436
Script:
#!/bin/bash
# Check the exact dependency configuration in the identified files
for file in $(rg -l '"@opentiny/vue-directive":'); do
echo "=== $file ==="
grep '"@opentiny/vue-directive":' "$file"
done
Length of output: 2746
packages/vue/src/tree-menu/src/props.ts (1)
2-2: LGTM! Import organization looks good.
The import of $props from @opentiny/vue-common is appropriate for props composition.
packages/theme-saas/src/date-range/index.less (1)
70-71: LGTM! Improved layout flexibility.
The changes enhance the header's layout by:
- Using auto margins for better centering
- Adding consistent line height with Tailwind's
leading-5
examples/sites/package.json (1)
90-90: Unable to review the dependency changes
The annotated code only shows a closing brace, while the AI summary mentions removal of @opentiny/vue-mobile. Without seeing the actual changes, I cannot provide a meaningful review of the dependency modifications.
packages/vue/src/grid/src/radio/src/radio.ts (2)
25-25: LGTM: Dependencies and directive registration
The imports and directive registration are properly structured for both Vue 2 and Vue 3 compatibility.
Also applies to: 27-27, 39-39
99-99: LGTM: Proper slot handling in render
The conditional rendering of labelContent based on slot existence is correct.
packages/vue/src/grid/src/table/src/utils/handleGlobalMousedownEvent.ts (1)
79-85: Verify the impact on table selection behavior
The new condition for clearing active state might affect table selection behavior when clicking between header and content areas.
packages/vue/src/tree-menu/src/pc.vue (1)
107-108: Verify icon usage after removing iconSearch import.
The removal of iconSearch import suggests changes in icon handling. Please verify that search functionality still works correctly with the new icon implementation.
✅ Verification successful
The removal of iconSearch import is safe as it's now imported through props
The iconSearch import was moved to the props.ts file where it's properly handled as a default value for the search icon prop. This is a good refactoring practice as it centralizes prop definitions and their default values.
- The search functionality remains intact as verified by:
- iconSearch is properly imported and used in
packages/vue/src/tree-menu/src/props.ts - It's set as the default value for the search icon prop:
default: () => iconSearch()
- iconSearch is properly imported and used in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for iconSearch usage in the codebase
rg "iconSearch" --type vue
Length of output: 54
Script:
#!/bin/bash
# Search for iconSearch usage with ripgrep, without file type filter
rg "iconSearch"
# Also check for any search-related icon imports
rg "icon.*Search" -A 2
# Look for template usage of search icons
rg "<.*search.*icon" -i
Length of output: 40978
packages/theme-saas/src/picker/index.less (1)
149-149: LGTM: Text size standardization improves consistency.
The changes standardize text sizes from text-xs to text-sm across different picker variants, which improves visual consistency.
Also applies to: 153-153, 241-241
packages/renderless/src/flowchart/index.ts (1)
293-297: LGTM! Good fix for the browser zoom issue.
The fix correctly handles the browser zoom discrepancy by adjusting the graphWidth value. The comment clearly explains the rationale, and the code includes proper bounds checking to prevent negative values.
Let's verify the impact of this change on different zoom levels:
✅ Verification successful
The graphWidth adjustment is safe and doesn't cause cascading issues
The code search reveals that the graphWidth adjustment is properly isolated:
- The adjustment only happens in the refresh function where the browser zoom issue is handled
- Other usages of graphWidth work with the adjusted value:
- In layout calculations (
afterConfig.layout) - In width calculations considering minWidth (
graph.width) - In the Vue component's resize handling
- In layout calculations (
The fix is well-contained and the adjusted value is consistently used throughout the flow, making it a safe change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other graphWidth usages to ensure the fix doesn't cause issues elsewhere
# Check for other graphWidth references
rg -A 3 "graphWidth" --type ts
Length of output: 2725
packages/vue/src/file-upload/src/mobile-first.vue (1)
676-676: LGTM! Improved conditional rendering logic.
The change correctly prevents the upload component from rendering when in display-only mode while maintaining proper slot handling.
| // 表头布局从block改为了flex，此处兼容插槽文本溢出不显示省略号问题 | ||
| &:not(:has(.tiny-grid-cell-text)) > span:only-of-type { | ||
| @apply text-ellipsis; | ||
| @apply overflow-hidden; | ||
| } |
There was a problem hiding this comment.
Consider browser compatibility for :has() selector
While the fix for the text overflow issue is elegant, the :has() selector has limited browser support.
Consider adding a fallback solution or using a more widely supported approach:
- &:not(:has(.tiny-grid-cell-text)) > span:only-of-type {
+ // Fallback solution
+ &:not(.has-cell-text) > span:only-of-type {
@apply text-ellipsis;
@apply overflow-hidden;
}Also, add a class toggle in the component's mounted hook:
mounted() {
const hasCellText = this.$el.querySelector('.tiny-grid-cell-text');
if (!hasCellText) {
this.$el.classList.add('has-cell-text');
}
}
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
Improvements
Refactor
Bug Fixes