fix: buildinCompStyle compact nested scenario #1090
Conversation
修复内置 Canvas行列容器组件在嵌套场景下，样式会发生透传的 bug
WalkthroughThe changes update several Vue components to apply dynamic styling directly within the template rather than relying on static CSS in the Changes
Sequence Diagram(s)sequenceDiagram
participant Parent
participant Component
participant Template
Parent->>Component: Pass style-related props
Component->>Component: Compute dynamic style object
Component->>Template: Bind computed styles inline (via :style or :styles)
Template->>Browser: Render element with updated styles
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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
bug
![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 (6)
packages/builtinComponent/src/components/CanvasCol.vue (2)
21-54: Consider enhancing prop validation and documentation.While the props are well-structured, consider these improvements:
- Add validator functions for
alignandjustAlignto ensure only valid values fromalignMapandjustAlignMapare accepted.- Add JSDoc comments to document the expected values and usage of each prop.
Example enhancement:
const props = defineProps({ flexBasis: { type: String, + /** CSS value for flex-basis (e.g., '100px', '50%') */ default: '0px' }, align: { type: [String, Number], + /** @type {keyof typeof alignMap} Alignment value */ default: '', + validator: (value) => !value || value in alignMap }, // ... similar improvements for other props })
66-72: Consider memoizing the getFlex function.The
getFlexfunction is called on every computation of thestylesobject. Consider memoizing it or moving it into the computed property to prevent unnecessary recalculations.-const getFlex = () => { +const computedFlex = computed(() => { const { flexBasis, grow, shrink, widthType } = props if (widthType === 'fixed') { return `0 0 ${getStyleValue(flexBasis)}` } return `${Number(grow)} ${Number(shrink)} ${getStyleValue(flexBasis)}` -} +}) const styles = computed(() => ({ - flex: getFlex(props.flexBasis), + flex: computedFlex.value, rowGap: getStyleValue(props.rowGap), // ... }))packages/builtinComponent/src/components/CanvasRow.vue (3)
2-11: Consider extracting complex inline styles to a class-based approach.While the dynamic styles effectively solve the nested style transmission issue, consider using Vue's class bindings for better maintainability. This could be achieved by defining base styles in CSS and using dynamic classes for variations.
Example approach:
- <div - class="canvas-row" - :style="{ - 'min-height': styles.minHeight, - 'row-gap': styles.rowGap, - 'column-gap': styles.colGap, - 'align-items': styles.align, - 'justify-content': styles.justAlign - }" - > + <div + class="canvas-row" + :class="styleClasses" + :style="{ 'min-height': styles.minHeight }" + >
20-41: Add prop documentation and validation.The props would benefit from:
- JSDoc comments explaining each prop's purpose and expected values
- Validation for alignment values
- More specific default values where applicable
Example enhancement:
const props = defineProps({ minHeight: { type: [String, Number], + validator: (value) => !value || typeof value === 'string' || typeof value === 'number', default: '' }, align: { type: [String, Number], + validator: (value) => !value || Object.keys(alignMap).includes(String(value)), default: 'stretch' }, // ... similar changes for other props })
1-59: Overall implementation effectively solves the nested style transmission issue.The transition to dynamic styles successfully addresses the core issue while maintaining component flexibility. The implementation is solid, though there's room for improvement in documentation and maintainability.
Consider documenting the rationale for choosing dynamic inline styles over other approaches in the component's documentation, as this architectural decision impacts the entire component system.
packages/builtinComponent/src/components/CanvasRowColContainer.vue (1)
16-21: Consider adding prop validation.The
rowGapprop accepts both string and number types with an empty string default. Consider adding validation to ensure valid CSS values:rowGap: { type: [String, Number], default: '', + validator: (value) => { + if (typeof value === 'number') return true + if (typeof value === 'string') { + return value === '' || /^-?\d*\.?\d+(px|rem|em|%|vh|vw)$/.test(value) + } + return false + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/builtinComponent/src/components/CanvasCol.vue(1 hunks)packages/builtinComponent/src/components/CanvasFlexBox.vue(1 hunks)packages/builtinComponent/src/components/CanvasRow.vue(1 hunks)packages/builtinComponent/src/components/CanvasRowColContainer.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (10)
packages/builtinComponent/src/components/CanvasCol.vue (2)
2-12: LGTM! Moving styles to inline bindings helps prevent style inheritance issues.The transition from CSS variables to inline style bindings is a good approach for fixing the style transmission issues in nested scenarios. The style properties are properly bound and follow Vue.js best practices.
75-81: LGTM! Clean separation of static and dynamic styles.The style section appropriately retains only the essential flex container properties while moving all dynamic styles to inline bindings. This separation helps prevent style inheritance issues in nested scenarios.
packages/builtinComponent/src/components/CanvasFlexBox.vue (3)
16-50: LGTM! Clean implementation following Vue 3 best practices.The script setup implementation is well-structured with:
- Proper prop definitions with types and defaults
- Efficient use of computed properties
- Helper function for consistent style value formatting
51-59: LGTM! Style implementation aligns with PR objectives.The style section effectively addresses the nested component style transmission issue by:
- Using scoped styles with proper deep selectors
- Setting appropriate flex properties
- Ensuring nested containers receive correct width
1-60: Verify the fix in nested component scenarios.While the implementation looks correct, please ensure thorough testing of:
- Style transmission in deeply nested scenarios
- Dynamic style updates when props change
- Interaction with other canvas components in the hierarchy
packages/builtinComponent/src/components/CanvasRow.vue (2)
52-58: LGTM! Core flex container setup is well-defined.The static styles appropriately handle the essential flex container properties that don't need to be dynamic.
43-49: Verify helper function implementation.The computed styles rely on external helper functions. Let's verify their implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for helper function implementations echo "Searching for helper.js/ts file..." fd -e js -e ts -e vue helper cat $(fd -e js -e ts -e vue helper | head -n 1) echo "Searching for getStyleValue usage..." rg "getStyleValue" -A 2Length of output: 6011
Helper functions verified and working as expected.
- The
getStyleValuefunction appends "px" for numbers or numeric strings and passes through values with valid CSS units, returning an empty string otherwise.- Both
alignMapandjustAlignMapinclude appropriate values with fallback defaults in the computed property.packages/builtinComponent/src/components/CanvasRowColContainer.vue (3)
28-33: LGTM! Clean and focused styles.The styles are minimal and appropriately focused on layout. The removal of static
row-gapin favor of dynamic binding is a good improvement.
2-7: LGTM! Verify nested component behavior.The dynamic style binding implementation follows Vue's best practices. Since this PR aims to fix style transmission in nested scenarios, let's verify the component's behavior when nested.
Let's check how this component is used in nested scenarios:
14-14: Verify the helper function implementation.Let's check the implementation of
getStyleValueto ensure consistent style value formatting:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for getStyleValue implementation ast-grep --pattern 'function getStyleValue($_) { $$$ }'Length of output: 59
Let's run a new shell script to locate the helper file and inspect its contents, especially focusing on the
getStyleValueimplementation. This will help us verify if the helper function is implemented as expected.
🏁 Script executed:
#!/bin/bash # Locate the helper file within packages/builtinComponent/src/components fd "helper\\.(js|ts)$" packages/builtinComponent/src/components # If the file is found, print its contents for manual inspection. # Assuming the file is packages/builtinComponent/src/components/helper.js, display its content: cat packages/builtinComponent/src/components/helper.js || trueLength of output: 1333
Helper function
getStyleValuecorrectly implements style formatting.
- The function appends "px" to numeric values (both numbers and numeric strings without units), ensuring consistent styling.
- It passes through values that already include valid units.
- The fallback to an empty string when the value does not match any criteria is appropriate for avoiding invalid style declarations.
| <div | ||
| class="canvas-flex-box" | ||
| :styles="{ | ||
| 'flex-direction': styles.flexDirection, | ||
| gap: styles.gap, | ||
| padding: styles.padding, | ||
| 'align-items': styles.alignItems, | ||
| 'justify-content': styles.justifyContent | ||
| }" | ||
| > |
There was a problem hiding this comment.
Fix incorrect style binding directive.
The template uses :styles which is incorrect. Vue.js uses :style (singular) for style binding.
Apply this diff to fix the style binding:
<div
class="canvas-flex-box"
- :styles="{
+ :style="{
'flex-direction': styles.flexDirection,
gap: styles.gap,
padding: styles.padding,
'align-items': styles.alignItems,
'justify-content': styles.justifyContent
}"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div | |
| class="canvas-flex-box" | |
| :styles="{ | |
| 'flex-direction': styles.flexDirection, | |
| gap: styles.gap, | |
| padding: styles.padding, | |
| 'align-items': styles.alignItems, | |
| 'justify-content': styles.justifyContent | |
| }" | |
| > | |
| <div | |
| class="canvas-flex-box" | |
| :style="{ | |
| 'flex-direction': styles.flexDirection, | |
| gap: styles.gap, | |
| padding: styles.padding, | |
| 'align-items': styles.alignItems, | |
| 'justify-content': styles.justifyContent | |
| }" | |
| > |
| <div class="canvas-flex-box"> | ||
| <div | ||
| class="canvas-flex-box" | ||
| :styles="{ |
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
修复内置 Canvas行列容器组件在嵌套场景下，样式会发生透传的 bug
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