feat: modal&drawer support appendToMain and zIndex
#5091
Conversation
mynetfan
requested review from
anncwb,
vince292007 and
a team
as code owners
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces enhancements to various UI components by adding default placeholder properties 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 (
|
mynetfan
changed the title
appendToMain and zIndex
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetContent.vue (3)
30-33: LGTM! Consider adding JSDoc commentsThe default values are appropriate. Consider adding JSDoc comments to document the purpose and impact of these properties.
Add documentation like this:
interface SheetContentProps extends DialogContentProps { + /** Specifies where to mount the sheet content. Defaults to 'body' */ appendTo?: HTMLElement | string; class?: any; modal?: boolean; open?: boolean; side?: SheetVariants['side']; + /** Base z-index for the sheet and its overlay. Defaults to 1000 */ zIndex?: number; }
53-59: Consider incrementing overlay z-indexThe current implementation uses the same z-index for both the overlay and content. To ensure proper stacking, consider incrementing the overlay's z-index slightly.
Apply this change:
- <SheetOverlay v-if="open && modal" :style="{ zIndex }" /> + <SheetOverlay v-if="open && modal" :style="{ zIndex: zIndex + 1 }" />This ensures the overlay always appears above the content, preventing potential stacking context issues.
Line range hint
1-71: Document layout implicationsSince this component can now be mounted to different containers, consider:
- Documenting potential CSS stacking context implications
- Adding usage examples for different layout scenarios
- Noting any limitations when used within scrollable containers
This will help users understand how to properly implement the component in various layout contexts.
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogScrollContent.vue (1)
45-45: Consider consolidating z-index handlingThe component currently has two sources of z-index:
- Tailwind's
z-50class- Dynamic
:style="{ zIndex }"bindingThis could lead to confusion about which takes precedence.
Consider removing the
z-50class and handling all z-index through the prop:-'border-border bg-background relative z-50 my-8 grid w-full max-w-lg gap-4 border p-6 shadow-lg duration-200 sm:rounded-lg md:w-full', +'border-border bg-background relative my-8 grid w-full max-w-lg gap-4 border p-6 shadow-lg duration-200 sm:rounded-lg md:w-full',packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2)
71-75: Consider centralizing z-index managementWhile the implementation is correct, consider extracting z-index values to a shared constants file for better maintainability across components.
Example implementation:
// constants/zIndex.ts export const Z_INDEX = { DIALOG_OVERLAY: 1000, DIALOG_CONTENT: 1000, // ... other z-index values } as const;
Line range hint
1-105: Consider documenting stacking context guidelinesAs this is a core UI component, consider documenting guidelines for:
- Recommended z-index ranges for different types of overlays (modals, drawers, tooltips)
- Best practices for using the
appendToprop in different layout contexts- Potential gotchas when nesting dialogs or combining with other overlay components
This will help maintain consistency as the component library grows.
playground/src/views/examples/drawer/in-content-demo.vue (2)
10-13: Uncomment or remove the commented close functionalityThe commented
drawerApi.close()line creates ambiguity about the intended behavior. Either:
- Uncomment it if the drawer should close on confirm
- Remove the comment if the drawer should stay open
onConfirm() { message.info('onConfirm'); - // drawerApi.close(); + drawerApi.close(); },
17-20: Consider using i18n for text contentThe Chinese text in the template should be internationalized for better maintainability and multi-language support.
- <Drawer append-to-main title="基础抽屉示例" title-tooltip="标题提示内容"> + <Drawer append-to-main :title="t('drawer.basicExample')" :title-tooltip="t('drawer.titleTooltip')"> <template #extra> extra </template> - 本抽屉指定在内容区域打开 + {{ t('drawer.contentAreaNote') }} </Drawer>playground/src/views/examples/modal/in-content-demo.vue (2)
10-13: Uncomment or remove the commented close functionalitySimilar to the drawer implementation, the commented
modalApi.close()creates ambiguity.onConfirm() { message.info('onConfirm'); - // modalApi.close(); + modalApi.close(); },
17-24: Consider using width prop instead of classUsing a class for width makes the component less flexible. Consider using the built-in width prop for better maintainability and consistency.
<Modal append-to-main - class="w-[600px]" + :width="600" :title="t('modal.basicExample')" :title-tooltip="t('modal.titleTooltip')" > - 此弹窗指定在内容区域打开 + {{ t('modal.contentAreaNote') }} </Modal>packages/@core/base/shared/src/constants/globals.ts (1)
10-11: Maintain consistent documentation languageThe JSDoc comment should follow the same pattern as other comments in the file, using the
@zh_CNtag if needed.- /** 内容区域的组件ID */ + /** + * @zh_CN 内容区域的组件ID + * @en_US Main content area component ID + */packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
120-123: Consider enhancing zIndex documentation.While the type is correct, consider adding:
- Default value in JSDoc
- Description of stacking context implications
/** * 弹窗层级 + * @description Controls the stacking order of the modal in relation to other elements + * @default 1000 */ zIndex?: number;playground/src/views/examples/drawer/index.vue (1)
47-56: Consider a more systematic approach to z-index managementWhile the z-index handling works, hardcoding the value 199 for top placement could be fragile. Consider:
- Using a constant for the z-index value
- Implementing a z-index management system that automatically handles layering
+const TOP_DRAWER_Z_INDEX = 199; + function openInContentDrawer(placement: DrawerPlacement = 'right') { inContentDrawerApi.setState({ placement }); if (placement === 'top') { - inContentDrawerApi.setState({ zIndex: 199 }); + inContentDrawerApi.setState({ zIndex: TOP_DRAWER_Z_INDEX }); } else { inContentDrawerApi.setState({ zIndex: undefined }); } inContentDrawerApi.open(); }apps/web-ele/src/adapter/component/index.ts (1)
Line range hint
77-85: LGTM! Consider extracting the placeholder string key.The addition of a default localized placeholder improves user experience. The placement before spreading props allows for easy override when needed.
Consider extracting the placeholder key to a constant to maintain consistency and ease maintenance:
+const SELECT_PLACEHOLDER_KEY = 'ui.placeholder.select'; ApiSelect: (props, { attrs, slots }) => { return h( ApiSelect, { - placeholder: $t('ui.placeholder.select'), + placeholder: $t(SELECT_PLACEHOLDER_KEY), ...props, ...attrs,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
apps/web-antd/src/adapter/component/index.ts(2 hunks)apps/web-ele/src/adapter/component/index.ts(2 hunks)apps/web-ele/src/views/demos/form/basic.vue(0 hunks)apps/web-naive/src/adapter/component/index.ts(2 hunks)apps/web-naive/src/views/demos/form/basic.vue(0 hunks)docs/src/components/common-ui/vben-drawer.md(2 hunks)docs/src/components/common-ui/vben-modal.md(2 hunks)packages/@core/base/shared/src/constants/globals.ts(1 hunks)packages/@core/ui-kit/layout-ui/package.json(1 hunks)packages/@core/ui-kit/layout-ui/src/vben-layout.vue(3 hunks)packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts(4 hunks)packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue(8 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.ts(3 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.vue(7 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue(2 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogOverlay.vue(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogScrollContent.vue(3 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetContent.vue(2 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetOverlay.vue(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/sheet/sheet.ts(2 hunks)packages/effects/common-ui/src/components/page/page.vue(1 hunks)playground/src/adapter/component/index.ts(2 hunks)playground/src/views/examples/drawer/in-content-demo.vue(1 hunks)playground/src/views/examples/drawer/index.vue(4 hunks)playground/src/views/examples/form/basic.vue(0 hunks)playground/src/views/examples/modal/in-content-demo.vue(1 hunks)playground/src/views/examples/modal/index.vue(4 hunks)
💤 Files with no reviewable changes (3)
- apps/web-naive/src/views/demos/form/basic.vue
- playground/src/views/examples/form/basic.vue
- apps/web-ele/src/views/demos/form/basic.vue
✅ Files skipped from review due to trivial changes (1)
- packages/effects/common-ui/src/components/page/page.vue
🔇 Additional comments (38)
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetContent.vue (1)
18-18: Verify property name consistency with requirements
The PR objectives mention adding an appendToMain property, but the implementation uses appendTo. Please clarify if this is intentional or if the property name should be aligned with the requirements.
Let's check for any other occurrences of these property names:
Also applies to: 23-23
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetOverlay.vue (2)
Line range hint 1-6: Verify scroll locking behavior with different mounting points
The scroll lock implementation might need adjustment based on the mounting container. Please test scroll behavior when the overlay is mounted to both the main content area and body.
✅ Verification successful
Scroll locking implementation is correctly handled for all mounting scenarios
The scroll lock implementation is robust and properly handles different mounting scenarios:
- Uses
document.bodyas the lock target, which is the correct approach regardless of overlay mounting point - Properly manages scrollbar width compensation for both body and fixed elements
- Handles cleanup on unmount through
tryOnBeforeUnmount - Both sheet and dialog overlays are positioned absolutely with
inset-0, ensuring full viewport coverage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check scroll lock implementation across components
# Look for scroll lock usage patterns
rg -A 5 "useScrollLock"
# Check for scroll-related utilities or mixins
fd -e ts -e js -e vue -x grep -l "scroll"
Length of output: 6425
Script:
#!/bin/bash
# Let's examine the scroll lock implementation and its usage in overlays
# Check the scroll lock implementation
cat packages/@core/composables/src/use-scroll-lock.ts
# Check the overlay implementations to understand mounting points
cat packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetOverlay.vue
cat packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogOverlay.vue
Length of output: 2217
10-10: Verify overlay positioning with different parent containers
The change from fixed to absolute positioning aligns with the new appendToMain feature, but ensure the overlay covers the intended area when mounted to different containers.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient context about the overlay implementations and their usage:
Overlay positioning change is consistent with similar components
The change from fixed to absolute positioning is correct and consistent with the codebase:
DialogOverlayuses the sameabsolutepositioning pattern- The parent
SheetContentusesDialogPortalwith configurableappendTo, which handles the mounting context - The overlay's z-index is properly managed by the parent component which passes it as a style prop
The implementation aligns with the portal-based mounting strategy and matches the pattern used in dialog components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for parent container positioning and potential styling conflicts
# Search for parent components that might affect absolute positioning
ast-grep --pattern 'class="$_relative$_"'
# Check for other overlay implementations for consistency
rg -l 'class=".*absolute.*overlay.*"'
Length of output: 83
Script:
#!/bin/bash
# Let's try a more comprehensive search for overlay and positioning patterns
# Check for Sheet component structure and positioning
rg -A 5 -B 5 "SheetOverlay"
# Look for other overlay implementations
rg -A 3 "class=.*overlay"
# Search for parent container positioning in Sheet components
rg -A 3 "class=.*relative.*" --type vue
# Check Sheet component imports and usage
rg -l "SheetOverlay"
Length of output: 2982
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogOverlay.vue (2)
10-10: Verify the positioning behavior across different mounting scenarios.
The change from fixed to absolute positioning aligns with the new appendToMain feature, but we should ensure proper overlay behavior in both mounting scenarios (body vs content area).
Let's verify the related dialog components and their positioning behavior:
Consider the following architectural recommendations:
- Ensure the parent container has
position: relativewhen mounted to content area - Consider adding a CSS class prop to allow custom positioning if needed
- Document the positioning behavior in component documentation
✅ Verification successful
The positioning change is architecturally sound and properly implemented
The verification reveals that the positioning change is well-integrated with the dialog system:
- The
DialogContent.vuecomponent properly handles theappendToprop (defaulting to 'body') and usesDialogPortalfor mounting - The overlay's
absolutepositioning works in conjunction with the modal's z-index management (passed via props) - The parent
DialogContentcomponent maintains proper stacking context withz-indexand positioning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check related dialog components for positioning and z-index handling
# Search for dialog-related components and their positioning/z-index usage
ast-grep --pattern 'class="$_*(fixed|absolute)$_*"' packages/@core/ui-kit/shadcn-ui/src/ui/dialog/
rg -A 3 "style.*z-index" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/
Length of output: 174
Script:
#!/bin/bash
# Let's try a different approach to verify dialog components
# 1. First, let's see all Vue files in the dialog directory
fd "\.vue$" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/
# 2. Then, let's examine the content of these files for positioning and mounting logic
fd "\.vue$" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/ --exec cat {}
# 3. Let's also check for any related test files that might verify positioning
fd "\.spec\.ts$|\.test\.ts$" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/
# 4. Let's search for mounting-related props and logic
rg -l "appendTo|teleport" packages/@core/ui-kit/shadcn-ui/src/ui/dialog/
Length of output: 8813
10-10: Review z-index management across dialog components.
The removal of the hardcoded z-index (1000) suggests z-index management has been moved to parent components. This is a good architectural decision for better flexibility, but we should verify the stacking context is properly maintained.
✅ Verification successful
Based on the search results, I can now provide a final response about the z-index management across dialog components:
Z-index management is properly handled through a consistent layering system
The removal of the hardcoded z-index from DialogOverlay is intentional and aligns with the codebase's z-index management strategy:
- The parent DialogContent component manages z-index (default: 1000) and properly propagates it to both overlay and content
- Z-index values are configurable through props, allowing flexible stacking context management
- The layout system uses a lower z-index range (100-200) ensuring modals always appear above layout elements
- Tailwind config defines consistent z-index scale (100, 1000) used throughout the components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze z-index usage across modal/dialog components
# Check for z-index props and their usage
ast-grep --pattern 'props: {
$$$
zIndex: $_
$$$
}'
# Look for style bindings with z-index
ast-grep --pattern ':style="{
$$$
zIndex: $_
$$$
}"'
Length of output: 119
Script:
#!/bin/bash
# Let's try a broader search for z-index related patterns in dialog components
# Search for z-index in CSS/SCSS files
rg -i "z-index" --type css
# Search for z-index in Vue files
rg -i "z-index" --type vue
# Search for class definitions that might control z-index
rg -i "bg-overlay" --type css --type vue -A 2 -B 2
# Look for dialog/modal related components and their styles
fd -e vue -e css -e scss . | grep -i "dialog\|modal" | xargs cat
Length of output: 33840
Script:
#!/bin/bash
# Let's search for z-index related styles in tailwind classes and CSS files
# Search for bg-overlay class definition in tailwind config
rg -i "bg-overlay" --type css --type ts --type js
# Search for z-index management in the codebase
rg -i "z-index|zIndex" --type css --type ts --type js -A 2 -B 2
# Look specifically for dialog/modal related styles
rg -i "\.dialog|\.modal" --type css --type ts --type js -A 2 -B 2
Length of output: 21271
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogScrollContent.vue (3)
17-20: LGTM: Props definition is type-safe and follows best practices
The addition of the zIndex prop with a default value of 1000 is well-implemented using Vue's withDefaults for type safety.
35-36: Verify the impact of changing from fixed to absolute positioning
The change from fixed to absolute positioning for the overlay could affect its behavior in different layout contexts. Please ensure:
- The overlay still covers the entire viewport when needed
- The overlay remains properly positioned relative to its parent container
- No scrolling issues occur when the dialog is open
✅ Verification successful
The previous commands failed due to file type recognition issues. Let's try with more generic commands to search through all files and explicitly add Vue file type for ripgrep. This should help us find:
- Usage patterns of DialogScrollContent
- Classes using fixed/absolute positioning in Vue files
- Related dialog components and their positioning styles
Change from fixed to absolute positioning is appropriate
The codebase analysis reveals that the change from fixed to absolute positioning is consistent with the component's design:
- The
DialogOverlaycomponent (packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogOverlay.vue) also usesabsolutepositioning withinset-0, establishing a consistent positioning strategy - The parent dialog component handles the viewport-level positioning, while these inner components use absolute positioning for proper stacking within the dialog's context
- The
inset-0class ensures full coverage within its container, maintaining the overlay's functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing tests or usage patterns of DialogScrollContent
rg -l "DialogScrollContent" --type vue
# Look for potential layout issues in related components
ast-grep --pattern 'class="$_*(fixed|absolute)$_*"' --lang vue
Length of output: 238
Script:
#!/bin/bash
# Search for any existing tests or usage patterns of DialogScrollContent
rg -l "DialogScrollContent"
# Look for potential layout issues in related components
rg "class=.*?(fixed|absolute)" --type-add 'vue:*.vue' --type vue
# Find related dialog components and their positioning
fd -e vue -e ts -e js . | xargs rg "Dialog.*?(fixed|absolute|position)"
Length of output: 9987
Line range hint 1-70: Verify implementation of appendToMain property
The PR objectives mention adding an appendToMain property, but this file only implements the zIndex property. Please verify if:
- The
appendToMainimplementation is missing from this component - This component doesn't require the
appendToMainproperty
✅ Verification successful
Let me try a different search strategy to verify this.
Let me check the Dialog base component to understand if appendToMain is needed.
Based on the search results, I can now provide a final response:
The appendToMain property is not required for this component
The search results show that appendToMain is a property specific to the popup-ui components (modal.vue and drawer.vue) in a different UI system, not the shadcn-ui dialog components. The shadcn-ui dialog system uses Radix Vue's portal system for mounting, which handles positioning through DialogPortal component that's already properly implemented in the DialogScrollContent.vue. Therefore, the PR's implementation of just the zIndex property is correct and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for appendToMain implementation in related files
rg -A 5 "appendToMain" --type vue
# Look for other dialog/drawer components that might need this property
fd -e vue -e ts . | rg -l "(Dialog|Drawer)"
Length of output: 115
Script:
#!/bin/bash
# Search for Vue files containing Dialog components
fd ".*Dialog.*\.vue$"
# Search for any mentions of appendToMain in the codebase
rg "appendToMain"
# Search specifically in the ui-kit directory for dialog-related files
fd . "packages/@core/ui-kit" -e vue | grep -i "dialog"
Length of output: 2269
Script:
#!/bin/bash
# Check the base Dialog component implementation
cat "packages/@core/ui-kit/shadcn-ui/src/ui/dialog/Dialog.vue"
# Check if there are any PR or commit messages related to appendToMain
git log --grep="appendToMain"
Length of output: 452
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2)
69-69: LGTM! Portal implementation looks good
The DialogPortal correctly uses the new appendTo prop to control the mounting point of the dialog.
79-84: Verify absolute positioning behavior
The dialog content uses absolute positioning (absolute class) which might cause positioning issues depending on the parent container's positioning context. Ensure this works correctly when appended to different containers.
Let's check for potential positioning-related issues:
packages/@core/base/shared/src/constants/globals.ts (1)
11-11: Verify the constant usage across the codebase
Let's ensure this constant is properly used in all relevant components, particularly in the layout and modal/drawer implementations.
✅ Verification successful
Based on the search results, I can provide a final response as the verification is complete. The constant ELEMENT_ID_MAIN_CONTENT is properly used across the codebase in the following key components:
Constant usage verified across layout and modal/drawer implementations
- Layout implementation:
packages/@core/ui-kit/layout-ui/src/vben-layout.vue - Modal implementation:
packages/@core/ui-kit/popup-ui/src/modal/modal.vue - Drawer implementation:
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue
The constant is used consistently and there are no instances of hardcoded values of __vben_main_content being used directly in the codebase, indicating proper usage of the constant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the main content ID constant and string literal
echo "Checking constant usage:"
rg "ELEMENT_ID_MAIN_CONTENT" --type ts --type vue
echo -e "\nChecking for potential hardcoded values:"
rg "__vben_main_content" --type ts --type vue
Length of output: 307
Script:
#!/bin/bash
# Search for usage of the main content ID constant and string literal with correct file types
echo "Checking constant usage:"
rg "ELEMENT_ID_MAIN_CONTENT"
echo -e "\nChecking for potential hardcoded values:"
rg "__vben_main_content"
# List all potential Vue files to verify file extension
echo -e "\nListing Vue files:"
fd -e vue
Length of output: 30895
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/sheet.ts (2)
4-4: Verify z-index conflicts with other components
The explicit z-index of 1000 might conflict with other overlays or modals. Consider making this configurable through props like other components in the PR.
Consider extracting z-index values to a shared configuration to maintain consistent stacking order across components.
✅ Verification successful
Z-index value aligns with component hierarchy
The z-index of 1000 for the sheet component is consistent with other overlay components in the UI kit (context menu, select, tooltip, hover card, dropdown menu all use z-index: 1000). Only the lock screen (z-index: 2000) and nprogress bar (z-index: 1031) have higher values, which is appropriate for their use cases. The global CSS defines lower z-indices (1-2) for base elements and maximum z-index (2147483646) for special cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other z-index declarations that might conflict
rg -n "z-\[(9|10)\d{2,}\]" --type css --type typescript
Length of output: 92
Script:
#!/bin/bash
# Search for z-index declarations with a broader pattern and without file type restrictions
rg -n "z-\[.*\]"
Length of output: 5095
15-15: Verify impact of removing h-full
The removal of h-full from the right variant might affect the sheet's height behavior. Please ensure this doesn't break existing layouts.
✅ Verification successful
Based on the search results, I can see that the Sheet component is primarily used in the Drawer component implementation, where the right-positioned sheet is used with other styling classes like flex w-[520px] flex-col and conditional classes for mobile/placement adjustments. The removal of h-full from the right variant appears to be compensated by:
- The
inset-y-0class which sets both top and bottom to 0, effectively making it full height - The parent Drawer component's flex column layout and additional height controls
Therefore, I can provide the final response:
Removal of h-full is safe as height is properly handled by other classes
The height of the right-positioned sheet is correctly managed through:
inset-y-0class which provides full viewport height anchoring- Parent Drawer component's flex column layout
- Additional responsive classes for mobile/placement adjustments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of right-positioned sheets
ast-grep --pattern 'side="right"'
Length of output: 35
Script:
#!/bin/bash
# Search for Sheet component usage with right side positioning using different syntax patterns
rg -U "Sheet.*side.*right" -A 5 -B 5
# Also search for SheetContent with right side positioning
rg -U "SheetContent.*side.*right" -A 5 -B 5
# Look for any direct usage of the sheet class variants
rg "data-\[state=closed\]:slide-out-to-right" -A 5 -B 5
Length of output: 1309
Script:
#!/bin/bash
# Search for actual Sheet component usage in the codebase
rg -U "<Sheet" -A 10 -B 2
# Also search for any test files that might be testing the Sheet component
fd -e test.tsx -e test.ts -x cat {}
Length of output: 69481
packages/@core/ui-kit/layout-ui/package.json (1)
43-43: LGTM! Dependency addition is appropriate
Adding @vben-core/shared as a workspace dependency makes sense for sharing common utilities across components.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (1)
Line range hint 72-109: LGTM! Well-documented properties
The new properties (modal, showCancelButton, showConfirmButton, zIndex) are well-documented and align with the PR objectives. The addition of these configuration options improves component flexibility.
✅ Verification successful
Properties are consistently implemented across modal and drawer components
The verification confirms that the properties modal, showCancelButton, showConfirmButton, and zIndex are properly implemented in both the modal and drawer components with consistent behavior and documentation:
- Both components define these properties in their respective interfaces (
ModalPropsandDrawerProps) - The properties have identical default values and documentation
- The implementation is symmetric between modal and drawer components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent property names across modal and drawer components
rg -A 2 "appendToMain|showCancelButton|showConfirmButton" --type typescript
Length of output: 112
Script:
#!/bin/bash
# Search for property definitions in TypeScript and JavaScript files
rg -t js -t ts "showCancelButton|showConfirmButton|modal" -A 2
Length of output: 43211
Script:
#!/bin/bash
# Find all TypeScript/JavaScript files that might contain drawer or modal components
fd -e ts -e js -e tsx -e jsx drawer
fd -e ts -e js -e tsx -e jsx modal
Length of output: 600
Script:
#!/bin/bash
# Use ast-grep to find interface or type definitions containing these properties
ast-grep --pattern 'interface $_ {
$$$
showCancelButton?: $_
$$$
}'
ast-grep --pattern 'interface $_ {
$$$
showConfirmButton?: $_
$$$
}'
Length of output: 30136
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
6-10: LGTM! Well-documented property addition.
The appendToMain property is properly documented with a clear purpose and default value.
docs/src/components/common-ui/vben-drawer.md (1)
77-77: LGTM! Documentation matches implementation.
The new properties are well-documented in the Props table with correct types and default values.
Also applies to: 99-99
playground/src/views/examples/modal/index.vue (3)
20-23: LGTM! Clean modal setup.
The modal setup follows the established pattern and properly connects to the demo component.
108-111: LGTM! Clear example implementation.
The card provides a clear example of the appendToMain feature with appropriate description.
12-12: Verify consistent property usage across components.
Let's ensure the new properties are consistently implemented across Modal and Drawer components.
Also applies to: 20-23
✅ Verification successful
Properties are consistently implemented across components
The verification shows that both Modal and Drawer components have consistent implementation of:
appendToMainproperty in Modal interfacezIndexproperty in both Modal and Drawer interfaces with the same type (number)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of appendToMain and zIndex properties
# Check Modal implementation
echo "Checking Modal implementation..."
ast-grep --pattern 'interface ModalProps {
$$$
appendToMain?: boolean;
$$$
}'
# Check Drawer implementation
echo "Checking Drawer implementation..."
ast-grep --pattern 'interface DrawerProps {
$$$
appendToMain?: boolean;
$$$
}'
# Check for zIndex property
echo "Checking zIndex implementation..."
rg "zIndex.*number" -A 2
Length of output: 11362
docs/src/components/common-ui/vben-modal.md (1)
Line range hint 83-110: Documentation updates look good!
The new properties appendToMain and zIndex are well documented with:
- Clear descriptions
- Proper type information
- Default values
- Consistent formatting with other properties
playground/src/adapter/component/index.ts (1)
88-88: Implementation of default placeholders looks good!
The changes:
- Add consistent default placeholders using i18n
- Allow override through props/attrs
- Follow the existing pattern used by other components
Also applies to: 103-103
apps/web-antd/src/adapter/component/index.ts (2)
87-87: Implementation of default placeholders looks good!
The changes:
- Add consistent default placeholders using i18n
- Allow override through props/attrs
- Follow the existing pattern used by other components
Also applies to: 102-102
Line range hint 87-93: Verify the different event name for ApiSelect
The visibleEvent prop is set to 'onDropdownVisibleChange' here but 'onVisibleChange' in the playground adapter. This difference might cause inconsistent behavior.
apps/web-naive/src/adapter/component/index.ts (2)
Line range hint 75-81: LGTM: ApiSelect placeholder implementation
The addition of a default placeholder using translations is a good practice for improving user experience.
Line range hint 88-97: LGTM: ApiTreeSelect placeholder implementation
The implementation correctly sets the placeholder and necessary tree-select specific properties.
playground/src/views/examples/drawer/index.vue (1)
124-147: LGTM: Clear example implementation
The example implementation provides a clear demonstration of the appendToMain feature with different placement options.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (4)
35-37: LGTM: Props implementation
The props are well-defined with appropriate default values.
119-121: LGTM: Dynamic append-to implementation
The computed property cleanly handles the dynamic target based on the appendToMain prop.
Line range hint 130-140: LGTM: SheetContent implementation
The component correctly applies the appendToMain and zIndex properties.
26-26: Verify documentation coverage
Ensure that the appendToMain and zIndex properties are properly documented in the component's documentation.
✅ Verification successful
Documentation is properly covered
Both appendToMain and zIndex properties are well documented in the component documentation:
- Found in
docs/src/components/common-ui/vben-drawer.mdwith proper descriptions and default values - Also documented in
docs/src/components/common-ui/vben-modal.mdfor consistency across components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new properties are documented
rg -A 5 "appendToMain|zIndex" "docs/"
Length of output: 1883
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (3)
36-36: LGTM: Default value for appendToMain is appropriately set
The default value of false for appendToMain maintains backward compatibility with existing behavior.
168-170: LGTM: Clean implementation of appendToMain logic
The getAppendTo computed property correctly handles the conditional mounting of the modal based on the appendToMain prop.
180-180: LGTM: Proper integration with DialogContent component
The append-to binding correctly utilizes the computed property to determine the mounting point.
packages/@core/ui-kit/layout-ui/src/vben-layout.vue (2)
461-462: LGTM: Clean constant definition
The idMainContent constant is appropriately defined using the imported ELEMENT_ID_MAIN_CONTENT.
559-559: LGTM: Proper ID assignment to LayoutContent
The ID is correctly assigned to the LayoutContent component, enabling it as a valid mounting point for modals and drawers.
apps/web-ele/src/adapter/component/index.ts (2)
Line range hint 91-102: LGTM! Consistent with ApiSelect implementation.
The placeholder implementation maintains consistency with ApiSelect, which is good for maintainability.
The same suggestion about extracting the placeholder key to a constant applies here as well.
Line range hint 1-1: Verify completeness of PR changes.
The PR objectives mention adding an appendToMain property to Modal and Drawer components, but the changes shown only include placeholder additions to select components. Please ensure all intended changes are included in the PR.
Let's verify if the Modal and Drawer component changes exist in other files:
✅ Verification successful
PR changes are consistent with objectives
The review comment's concern about missing appendToMain property changes is incorrect. The code analysis shows that the appendToMain property has been properly added to both Modal and Drawer components:
- Modal component: Added
appendToMainproperty inpackages/@core/ui-kit/popup-ui/src/modal/modal.tswith proper implementation inmodal.vue - Drawer component: Added
appendToMainproperty inpackages/@core/ui-kit/popup-ui/src/drawer/drawer.tswith proper implementation indrawer.vue
Both components have the property properly documented, defaulted to false, and implemented with the functionality to append to main content area when enabled. The PR changes align with its stated objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Modal and Drawer component definitions and their usage of appendToMain
rg -l "Modal|Drawer" | xargs ast-grep --pattern 'appendToMain'
# Search for recent changes to Modal and Drawer components
git diff HEAD~10 -G "Modal|Drawer" --stat
Length of output: 436
Script:
#!/bin/bash
# Let's check the actual content of these changed files to see if they include appendToMain property
rg -A 5 -B 5 "appendToMain" playground/src/views/examples/drawer/ playground/src/views/examples/modal/
# Also check for Modal and Drawer component definitions
ast-grep --pattern 'const $_Modal = defineComponent({
$$$
})'
ast-grep --pattern 'const $_Drawer = defineComponent({
$$$
})'
Length of output: 235
Script:
#!/bin/bash
# Let's try a broader search for Modal and Drawer components in the source code
rg -l "Modal|Drawer" apps/
# And check their content
fd -t f . apps/ -e ts -e vue | xargs rg -l "Modal|Drawer" | xargs cat
# Also check for any props or interfaces related to appendToMain
rg -A 5 "interface.*Props" apps/
Length of output: 11134
Script:
#!/bin/bash
# Let's check the package source code since Modal and Drawer might be in a shared package
fd -t f . packages/ -e ts -e vue | xargs rg -l "Modal|Drawer" | xargs cat
# Also check for any recent changes in packages directory
git diff HEAD~10 -G "Modal|Drawer" packages/ --stat
Length of output: 68065
Script:
#!/bin/bash
# Let's check the source code for Modal and Drawer components in packages directory
fd -t f . packages/ -e ts -e vue | xargs rg -l "appendToMain"
# Also check for any recent changes in packages directory related to Modal and Drawer
git diff HEAD~10 packages/ | rg -A 5 -B 5 "appendToMain"
Length of output: 2187
mynetfan
force-pushed
the
feat/modal-drawer-append-to-main
branch
from
87755bb to
6e88d61
Compare
|
尚有问题需要解决： |
mynetfan
changed the title
appendToMain and zIndexappendToMain and zIndex
mynetfan
changed the title
appendToMain and zIndexappendToMain and zIndex
Description
为
Modal和Drawer新增appendToMain属性，可以配置将弹窗挂载到body还是layout的内容区域。Modal:
Drawer:
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
Release Notes
New Features
ApiSelectandApiTreeSelectcomponents.InContentDrawerandInContentModal) with enhanced functionality.Bug Fixes
ApiSelectandApiTreeSelectin specific forms, improving user input clarity.Documentation
Vben DrawerandVben Modalto include new properties likeappendToMainandzIndex.Chores