fix(drawer): [drawer] modify height props,setting the height of the left and right sliders #2484
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
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 (
|
James-9696
changed the title
![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 (15)
examples/sites/demos/pc/app/drawer/dragable-composition-api.vue (4)
13-13: Consider making the height more flexible for this demoWhile setting a fixed height of
600pxprovides consistency, it might limit the demonstration value of this dragable drawer example. Since this is specifically a demo for height/width dragging functionality, consider:
- Using a more responsive initial height (like '100vh')
- Adding a comment explaining the rationale for the fixed height
- height="600px" + :height="placement === 'bottom' ? '100vh' : '600px'"
Line range hint
15-19: Enhance the drag interaction instructionsSince this is a demo component, the instructions could be more detailed to better guide users on the drag functionality.
<div> - <p v-if="placement === 'right'">横向拖拽左边框可改变抽屉主体宽度。</p> - <p v-else>竖向拖拽上边框可改变抽屉主体高度。</p> + <p v-if="placement === 'right'"> + 横向拖拽左边框可改变抽屉主体宽度。当前宽度可拖拽调整。 + </p> + <p v-else> + 竖向拖拽上边框可改变抽屉主体高度。初始高度为600px，可拖拽调整。 + </p> </div>
Line range hint
31-33: Consider enhancing the drag event feedbackThe current drag event handler only shows a modal message. Consider adding visual feedback or updating a reactive state to demonstrate the real-time dimensions.
+const currentDimensions = ref({ width: '', height: '' }) + const drag = ({ width, height }) => { + currentDimensions.value = { width, height } TinyModal.message({ message: `抽屉的宽为${width},高为${height}`, status: 'info' }) }Then display it in the template:
<div> <p v-if="placement === 'right'">横向拖拽左边框可改变抽屉主体宽度。</p> <p v-else>竖向拖拽上边框可改变抽屉主体高度。</p> + <p v-if="currentDimensions.width || currentDimensions.height"> + 当前尺寸: {{ currentDimensions.width || '默认' }} x {{ currentDimensions.height || '默认' }} + </p> </div>
Line range hint
1-40: Add documentation for the dragable featureSince this is a demo component showcasing the dragable functionality, it would be beneficial to add JSDoc comments explaining the feature and its usage.
Add this documentation at the top of the file:
+/** + * @description 抽屉拖拽示例 + * 演示了Drawer组件的可拖拽功能: + * 1. 宽度拖拽 - 从右侧打开的抽屉可以通过拖拽左边框调整宽度 + * 2. 高度拖拽 - 从底部打开的抽屉可以通过拖拽上边框调整高度 + * @example + * <tiny-drawer dragable height="600px" @drag="onDrag"> + * <!-- drawer content --> + * </tiny-drawer> + */examples/sites/demos/pc/app/drawer/placement-composition-api.vue (2)
13-13: Consider extracting magic numbers into constants.While the height value of
600pxis valid, it would be more maintainable to define these dimensions as constants, especially since they're used across multiple drawer examples.<script setup> import { ref } from 'vue' import { TinyDrawer, TinyButton } from '@opentiny/vue' + +const DRAWER_HEIGHTS = { + SIDE: '600px', + TOP_BOTTOM: '400px' +} const visible = ref(false) const placement = ref('right')Then update the template:
- height="600px" + :height="DRAWER_HEIGHTS.SIDE"
Based on the analysis of the codebase, I can now provide the final response:
Fixed heights in drawer examples need responsive alternatives
The review comment's concerns about fixed heights are valid. The analysis reveals:
- All drawer examples consistently use fixed pixel heights (400px/600px)
- No examples demonstrate responsive behavior despite the framework having window size breakpoint detection capabilities
- No viewport-relative or percentage-based units are currently used
Recommendations:
- Add examples showing responsive drawer heights using viewport units (vh) or percentages
- Include a responsive demo utilizing the framework's built-in breakpoint detection
- Document height recommendations for different screen sizes in the examples
🔗 Analysis chain
Line range hint
1-40: Verify drawer height behavior across different screen sizes.The implementation uses different fixed heights for different placements:
- Left/Right: 600px
- Top/Bottom: 400px
- Default (from context): 100vh
While this makes sense for demo purposes, we should ensure these values work well across different viewport sizes.
Consider the following recommendations:
- For left/right drawers, using a percentage or viewport-relative unit might be more responsive than a fixed pixel value
- Document the reasoning behind these specific height values in the code comments
- Consider adding responsive variants to demonstrate how the drawer adapts to different screen sizes
Let me check the actual height configurations in these files to better understand the patterns and variations.
Let me check if there are any responsive or viewport-relative units being used in the drawer examples, as this would help understand if there are already some responsive patterns in place.
Let me check the drawer component's source code to see if it has any built-in responsive behavior or default height handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other height-related configurations in drawer examples rg -l "height.*px" "examples/sites/demos/pc/app/drawer/"Length of output: 414
Script:
#!/bin/bash # Get the height configurations from all drawer examples rg "height.*px" "examples/sites/demos/pc/app/drawer/" -B 2 -A 2Length of output: 3570
Script:
#!/bin/bash # Search for viewport-relative units (vh, vw) or percentage values in drawer examples rg "height.*[vh|vw|%]" "examples/sites/demos/pc/app/drawer/" # Also check if there are any media queries or responsive utilities rg "@media|responsive" "examples/sites/demos/pc/app/drawer/"Length of output: 442
Script:
#!/bin/bash # Find the drawer component's source file fd -e vue -e ts drawer$ packages/ # Also check for any documentation about drawer sizing rg -i "drawer.*height|size|responsive" "packages/" -g "*.md"Length of output: 524
examples/sites/demos/pc/app/drawer/dragable.vue (2)
16-18: Consider improving accessibility and internationalizationThe demo's text content is in Chinese and lacks proper accessibility considerations for the drag functionality.
Consider these improvements:
- Add ARIA attributes for drag handles
- Add visual indicators for draggable areas
- Internationalize the text content
Example implementation:
<div> - <p v-if="placement === 'right'">横向拖拽左边框可改变抽屉主体宽度。</p> - <p v-else>竖向拖拽上边框可改变抽屉主体高度。</p> + <p v-if="placement === 'right'" role="tooltip" aria-label="Drag left border to resize drawer width"> + <i class="drag-indicator"></i> {{ t('drawer.dragWidthHint') }} + </p> + <p v-else role="tooltip" aria-label="Drag top border to resize drawer height"> + <i class="drag-indicator"></i> {{ t('drawer.dragHeightHint') }} + </p> </div>
Line range hint
39-41: Enhance drag event feedbackThe current drag event feedback only shows a modal message, which might not provide the best user experience.
Consider adding real-time visual feedback:
drag({ width, height }) { - TinyModal.message({ message: `抽屉的宽为${width},高为${height}`, status: 'info' }) + // Only show dimensions while actively dragging + this.dimensions = `${width} × ${height}` + // Debounce modal message for final dimensions + this.debouncedShowModal(() => { + TinyModal.message({ + message: `Drawer dimensions: ${width} × ${height}`, + status: 'info' + }) + }) }examples/sites/demos/pc/app/drawer/placement.vue (1)
12-12: Consider documenting the rationale for fixed heights.While adding a fixed height is valid, the choice of different fixed heights (600px for left/right vs 400px for top/bottom) seems arbitrary and could benefit from documentation explaining the design decisions.
packages/vue/src/drawer/src/index.ts (2)
Line range hint
73-73: Enhance type safety and documentation for customSlots property.The
customSlotsproperty needs better type definition and documentation:Consider applying these improvements:
- // tiny 新增：用于方法调用组件 - customSlots: Object + // Defines custom slots for programmatically created drawers + customSlots: { + type: Object as PropType<Record<string, Slot>>, + default: () => ({}), + validator: (value: Record<string, unknown>) => { + // Add validation for required slot names if any + return true + } + }Don't forget to:
- Add corresponding type imports
- Document supported slot names and their expected content
- Add usage examples in the component documentation
Line range hint
1-73: Consider enhancing the drawer's responsive design system.To improve the drawer component's architecture:
- Consider introducing responsive breakpoints system for height/width
- Add support for CSS custom properties for easier theming
- Implement a more robust slot typing system using TypeScript template literal types
Example approach:
type DrawerPlacement = 'left' | 'right' | 'top' | 'bottom'; type DrawerSlotName = 'header' | 'footer' | 'default'; type ResponsiveValue<T> = T | Record<'sm' | 'md' | 'lg', T>; interface DrawerProps { placement: DrawerPlacement; height: ResponsiveValue<string>; slots: Partial<Record<DrawerSlotName, Slot>>; }packages/vue/src/drawer/src/pc.vue (3)
Line range hint
156-156: Consider adding type definition and documentation for dragable prop.The
dragableprop has been added, but it would benefit from:
- Type definition for better TypeScript support
- Default value specification
- Documentation of its purpose and behavior
Consider enhancing the prop definition:
'dragable': { + type: Boolean, + default: false, + description: 'Enable dragging functionality for the drawer' }
34-34: Document the height behavior changes in release notes.The combination of:
- Applying height to all placements
- Changing default height from '500px' to '100vh' (as per AI summary)
represents a significant change in the drawer's default behavior.Consider:
- Adding a migration guide for users upgrading from previous versions
- Documenting the new responsive behavior in component documentation
- Adding examples demonstrating the height behavior with different placements
34-34: Add tests for new height behavior.The PR objectives indicate that tests haven't been added. Please add tests covering:
- Height behavior with different placements
- Interaction between dragable prop and height
- Default height (100vh) behavior
Would you like help creating these test cases? I can provide a template for the test suite.
examples/sites/demos/apis/drawer.js (1)
Line range hint
175-184: Consider enhancing the documentation for height behavior.While the basic description is present, it would be helpful to:
- Document that the default value uses viewport units (100vh)
- Add examples of common height configurations
- Note any considerations for mobile viewports
Would you like me to help draft the enhanced documentation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
examples/sites/demos/apis/drawer.js(1 hunks)examples/sites/demos/pc/app/drawer/dragable-composition-api.vue(1 hunks)examples/sites/demos/pc/app/drawer/dragable.vue(1 hunks)examples/sites/demos/pc/app/drawer/placement-composition-api.vue(1 hunks)examples/sites/demos/pc/app/drawer/placement.vue(1 hunks)packages/vue/src/drawer/src/index.ts(1 hunks)packages/vue/src/drawer/src/pc.vue(1 hunks)
🔇 Additional comments (5)
examples/sites/demos/pc/app/drawer/dragable.vue (1)
Line range hint 1-45: Verify height property implementation across demos
The changes to height handling need verification across the codebase to ensure consistency and proper documentation.
✅ Verification successful
Height property implementation is consistent across drawer demos
The verification shows that:
- The height property is consistently implemented across drawer demos with a default value of '100vh' defined in the component's constants
- The height property is properly used in both dragable and placement demos with explicit values (e.g., "600px", "400px")
- The height property is correctly handled in the drawer's renderless logic with proper fallback to default value
- The recent addition of height property is documented in the changelog
The implementation in the dragable demo aligns with the component's design and other demos' usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for height property usage across drawer demos
echo "Checking height property usage in drawer demos..."
rg -l "tiny-drawer" examples/sites/demos/pc/app/drawer/
# Check for documentation updates
echo "Checking for documentation updates..."
fd -e md -e vue . examples/sites/demos/pc/app/drawer/ --exec grep -l "height"
# Verify default height value
echo "Verifying default height value..."
rg "height.*100vh" src/drawer/
Length of output: 3788
Script:
#!/bin/bash
# Let's check the actual implementation and usage of height property in drawer demos
echo "Checking height property implementation in dragable demos..."
rg -A 5 "height.*=.*" examples/sites/demos/pc/app/drawer/dragable*.vue
echo "Checking height property implementation in placement demos..."
rg -A 5 "height.*=.*" examples/sites/demos/pc/app/drawer/placement*.vue
# Let's find the drawer component source
echo "Finding drawer component source..."
fd -t f -e vue drawer
# Once found, let's check its implementation
echo "Checking drawer component implementation..."
rg -A 5 "props.*height" .
Length of output: 16626
Script:
#!/bin/bash
# Let's check the drawer component's implementation in the packages directory
echo "Checking drawer component source..."
fd -t f drawer packages/
echo "Checking height property implementation in drawer component..."
rg -A 10 "props.*height|height.*props" packages/
# Check for any height-related constants
echo "Checking drawer constants..."
rg "DEFAULT_HEIGHT" packages/
Length of output: 24204
examples/sites/demos/pc/app/drawer/placement.vue (1)
12-12: Verify consistency with default height behavior.
The AI summary indicates that the drawer's default height was changed to '100vh' in drawer.js, but this example uses fixed pixel values. Consider if this example should demonstrate both fixed and viewport-relative heights for better documentation of the component's capabilities.
Consider adding another example that demonstrates:
- Using the default '100vh' height
- Responsive height values using CSS variables or viewport units
- Different height units (%, vh, rem) for various use cases
packages/vue/src/drawer/src/index.ts (1)
13-13: Verify the impact of changing default height to viewport units.
While changing to 100vh improves responsiveness, please consider:
- Mobile browser compatibility (especially iOS Safari's viewport height handling)
- Impact on existing implementations that might depend on the fixed 500px height
Let's verify the usage and potential impact:
✅ Verification successful
Based on the search results, I can now provide a final assessment:
Changing DEFAULT_HEIGHT to 100vh is safe and improves responsiveness
The change from '500px' to '100vh' for DEFAULT_HEIGHT is a valid improvement because:
-
The codebase shows the drawer component properly handles height through:
- Height prop with dynamic calculations for different placements
- Dragable height support
- Flexible height handling in both PC and mobile-first versions
-
No existing code depends on the fixed 500px height:
- No direct references to the 500px value were found
- The height is properly abstracted through props and computed values
- The component already supports dynamic heights
-
The change improves responsiveness:
- 100vh ensures the drawer takes full viewport height
- This is consistent with modern responsive design practices
- Both mobile and desktop views will benefit from viewport-relative sizing
The implementation properly handles height through props and computed values, making this constant change safe and beneficial.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing drawer height customizations that might be affected
rg -l "tiny-drawer.*height" --type html --type vue
# Check for existing mobile-specific handling
rg -l "mobile.*drawer" --type ts --type vue
Length of output: 154
Script:
#!/bin/bash
# Search for drawer height customizations with correct file types
rg -t html -t typescript -t javascript "tiny-drawer.*height"
# Check for mobile-specific handling
rg -t typescript -t javascript "mobile.*drawer"
# Look for drawer usage patterns
rg -t typescript -t javascript -t html "drawer"
# Find all Vue files that might use the drawer
fd -e vue
# Check for any height-related props or styles in Vue files
rg "height" -g "*.vue"
Length of output: 424384
Script:
#!/bin/bash
# Search for drawer height usage in tests and docs
rg -t typescript -t javascript -t markdown "DEFAULT_HEIGHT|drawer.*height" --glob '!dist/**' --glob '!node_modules/**'
# Look for drawer component usage patterns
rg -t vue -t typescript -t javascript "tiny-drawer" --glob '!dist/**' --glob '!node_modules/**'
Length of output: 279
Script:
#!/bin/bash
# Search for drawer height usage in tests and docs
rg "DEFAULT_HEIGHT|drawer.*height" -g "*.{ts,js,md}"
# Look for drawer component usage patterns
rg "tiny-drawer" -g "*.{vue,ts,js}"
# Search for height-related code in drawer component
rg "height" packages/vue/src/drawer/
Length of output: 16429
packages/vue/src/drawer/src/pc.vue (1)
34-34: Verify the impact of height application on left/right placements.
The height style is now being applied to all placement positions, including 'left' and 'right'. While this aligns with the PR's objective to modify height properties, we should ensure this doesn't cause unintended layout issues.
Let's verify the usage patterns in the codebase:
Consider documenting this behavior change in the component's documentation to help users understand how height is applied across different placements.
✅ Verification successful
The previous command failed due to file type recognition. Let's try with more generic patterns to find drawer usage with different placements, which will help us verify the impact of the height changes on left/right placements.
The search results show that there are demo and test files related to drawer placement. Let's examine their contents to understand how left/right placements are being used and tested, which will help us verify if the height changes are intentional and properly tested.
Height application on left/right placements is intentional and well-tested
Based on the comprehensive analysis of the codebase:
- The demo files (placement.spec.ts and placement.vue) show that left/right placements are core functionality and properly tested
- The drawer component already handles left/right placements with appropriate CSS classes and styles
- The height property application aligns with the component's existing behavior where dimensions are controlled based on placement
- The change is consistent with the documented behavior in webdoc files which show left/right as standard placement options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for drawer usage with left/right placements to assess impact
rg -l "placement.*['\"](?:left|right)['\"].*tiny-drawer" --type vue
Length of output: 101
Script:
#!/bin/bash
# Search for drawer usage with left/right placements using a more general pattern
rg -l "placement.*['\"](?:left|right)['\"].*drawer"
# Also search for any drawer component usage
rg -l "drawer.*placement"
# Let's also check the demo or example files
fd "demo|example" --type f | xargs rg -l "drawer.*placement"
Length of output: 295
Script:
#!/bin/bash
# Let's examine the content of the demo files to understand the usage patterns
cat examples/sites/demos/pc/app/drawer/placement.spec.ts
cat examples/sites/demos/mobile-first/app/drawer/placement.vue
# Also check the component's test files for placement-related tests
fd "drawer.*test|drawer.*spec" | xargs rg "placement.*(?:left|right)"
Length of output: 22041
examples/sites/demos/apis/drawer.js (1)
175-175: LGTM! Changing default height to viewport units improves responsiveness.
The change from fixed pixels to viewport height units is a good improvement for responsive design.
Let's verify the impact on existing implementations:
✅ Verification successful
Based on the search results, I can now generate the final response:
The change to '100vh' default height is safe and improves component flexibility
The codebase analysis shows that:
- The drawer component handles height dynamically through computed properties
- Both PC and mobile-first implementations support flexible height configurations
- Existing usages in demos primarily focus on width customization, with height being an optional property
- The component's internal implementation properly handles height for all placement options (top, bottom, left, right)
The change from '500px' to '100vh' default is backwards compatible since:
- Custom height values can still be passed to override the default
- The component's internal logic remains unchanged
- No existing implementations were found that depend on the '500px' default height
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom height configurations in drawer components
# that might be affected by the default height change
echo "Searching for drawer components with height configurations..."
rg -g '*.{vue,jsx,tsx}' -B 2 -A 2 'tiny-drawer.*height'
Length of output: 190
Script:
#!/bin/bash
# Let's try a broader search for drawer-related height configurations
# First, search for drawer components
echo "Searching for drawer components and their height configurations..."
rg -g '*.{vue,jsx,tsx,js}' -i "drawer.*height|height.*drawer" -B 2 -A 2
# Also search for any files containing drawer component usage
echo -e "\nSearching for files containing drawer component imports or usage..."
rg -g '*.{vue,jsx,tsx,js}' "(?i)(import.*drawer|<tiny-drawer|createDrawer)"
# Check the drawer component's source files
echo -e "\nSearching for height-related code in drawer implementation..."
fd -e js -e vue -e tsx drawer --exec rg -l "height|defaultProps|props"
Length of output: 13235
Script:
#!/bin/bash
# Let's examine the drawer implementation files for height-related configurations
echo "Checking drawer implementation files for height configurations..."
rg -g '*.{vue,js,tsx}' -B 3 -A 3 "height.*:|height[\"'].*:|defaultProps.*height" packages/vue/src/drawer/
# Also check for any height-related code in dragable drawer demos
echo -e "\nChecking dragable drawer demos for height usage..."
rg -g '*.{vue,js,tsx}' -B 3 -A 3 "height" examples/sites/demos/pc/app/drawer/dragable
# Look for any height-related props in the renderless implementation
echo -e "\nChecking renderless implementation for height props..."
rg -g '*.{js,ts}' -B 3 -A 3 "height" packages/vue-renderless/drawer/
Length of output: 1899
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
New Features
dragableproperty to enhance the drawer's functionality, enabling drag-and-drop capability.Bug Fixes
Chores