feat: modal&drawer support appendToMain and zIndex #5092
Conversation
mynetfan
requested review from
anncwb,
vince292007 and
a team
as code owners
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogOverlay.vueOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request introduces updates to the documentation and properties of several UI components, including the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 5
🧹 Outside diff range and nitpick comments (22)
packages/effects/common-ui/src/components/page/page.vue (1)
Line range hint
15-23: Document auto-content-height requirement for modal/drawer integrationSince this component's
autoContentHeightproperty is crucial for proper modal/drawer functionality when usingappendToMain, consider adding this requirement to the property's documentation./** * 根据content可见高度自适应 + * @remarks Required when using Modal or Drawer components with appendToMain property */ autoContentHeight?: boolean;packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2)
50-60: Consider simplifying and renaming the helper functionThe
isAppendToBodyfunction could be improved for clarity and maintainability:-function isAppendToBody() { - return ( - props.appendTo === 'body' || - props.appendTo === document.body || - !props.appendTo - ); +function shouldUseFixedPosition() { + return !props.appendTo || ['body', document.body].includes(props.appendTo); } const position = computed(() => - isAppendToBody() ? 'fixed' : 'absolute' + shouldUseFixedPosition() ? 'fixed' : 'absolute' );The suggested changes:
- Rename the function to better reflect its purpose
- Simplify the conditions using array inclusion
- Make the code more maintainable
Line range hint
1-117: Consider documenting mounting behavior and testing scenariosSince this component now supports flexible mounting locations, consider:
- Documenting the implications of different mounting strategies (body vs. main content)
- Adding examples in the component's documentation
- Creating test cases for different mounting scenarios
- Ensuring proper cleanup of mounted elements
This flexibility in mounting locations could affect:
- Event bubbling behavior
- Style inheritance
- Screen reader accessibility
- Portal cleanup on component destruction
Would you like help creating documentation templates or test scenarios for these cases?
playground/src/views/examples/drawer/in-content-demo.vue (3)
10-13: Clean up commented code in onConfirm handlerThe commented
drawerApi.close()line should either be removed or implemented based on the intended behavior.onConfirm() { message.info('onConfirm'); - // drawerApi.close(); },
17-20: Consider internationalizing the text contentThe component contains hardcoded Chinese text. Consider using i18n translations for better maintainability and internationalization 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') }}
17-17: Document append-to-main requirementConsider adding a comment explaining that
append-to-mainrequires the parentPagecomponent to haveauto-content-heightset.playground/src/views/examples/modal/in-content-demo.vue (3)
18-20: Consider using width prop instead of classUsing a class for width specification might override component's internal styling. Consider using the Modal's built-in width prop instead.
<Modal append-to-main - class="w-[600px]" + :width="600" title="基础弹窗示例"
10-13: Clean up commented code in onConfirm handlerThe commented
modalApi.close()line should either be removed or implemented based on the intended behavior.onConfirm() { message.info('onConfirm'); - // modalApi.close(); },
20-24: Consider internationalizing the text contentThe component contains hardcoded Chinese text. Consider using i18n translations for better maintainability and internationalization support.
- title="基础弹窗示例" - title-tooltip="标题提示内容" + :title="t('modal.basicExample')" + :title-tooltip="t('modal.titleTooltip')" > - 此弹窗指定在内容区域打开 + {{ t('modal.contentAreaNote') }}packages/@core/base/shared/src/constants/globals.ts (1)
10-12: LGTM! Consider enhancing the documentationThe constant follows the established patterns and naming conventions. Consider adding a more detailed JSDoc comment explaining its relationship with the
appendToMainfeature.-/** 内容区域的组件ID */ +/** + * ID for the main content area component. + * Used by Modal and Drawer components when appendToMain is enabled. + * @zh_CN 内容区域的组件ID + */packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogScrollContent.vue (1)
35-36: Consider extracting overlay styles to a shared classThe overlay has complex styling that could benefit from being extracted to a shared utility class, especially since similar styling might be needed for other dialog variants.
Consider creating a shared class in your CSS utilities:
-class="data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 border-border absolute inset-0 grid place-items-center overflow-y-auto border bg-black/80" +class="dialog-overlay"packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (1)
106-109: Consider enhancing zIndex documentationThe documentation for the
zIndexproperty could be improved by including the default value in the JSDoc comment, similar to other properties./** * 抽屉层级 + * @default 1000 */ zIndex?: number;packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
120-123: Consider enhancing zIndex documentationFor consistency with other properties and the Drawer component, consider adding the default value to the JSDoc comment.
/** * 弹窗层级 + * @default 1000 */ zIndex?: number;playground/src/views/examples/vxe-table/basic.vue (2)
70-78: Consider adding error handling to open functionsThe open functions could benefit from try-catch blocks to handle potential errors gracefully.
function openModal() { + try { modalApi.open(); + } catch (error) { + console.error('Failed to open modal:', error); + } } function openDrawer() { + try { drawerApi.open(); + } catch (error) { + console.error('Failed to open drawer:', error); + } }
93-98: Consider demonstrating new propertiesThe example could showcase the new
appendToMainandzIndexproperties to better demonstrate their usage.- <Modal title="弹窗测试"> + <Modal + title="弹窗测试" + :append-to-main="true" + :z-index="2000" + > <p>这是一个弹窗</p> </Modal> - <Drawer title="抽屉测试"> + <Drawer + title="抽屉测试" + :append-to-main="true" + :z-index="1900" + > <p>这是一个抽屉</p> </Drawer>docs/src/components/common-ui/vben-drawer.md (1)
77-77: Consider enhancing the zIndex property descriptionWhile the property definitions are accurate, the zIndex description could be more detailed to explain:
- Its relationship with other UI elements
- Common scenarios for customization
Also applies to: 99-99
playground/src/views/examples/drawer/index.vue (1)
47-56: Consider extracting z-index magic number into a constantThe z-index value of 199 is hardcoded. Consider extracting this into a named constant to improve maintainability and document the relationship with the layout's z-index of 200.
+const CONTENT_DRAWER_Z_INDEX = 199; // Below layout's z-index (200) + function openInContentDrawer(placement: DrawerPlacement = 'right') { inContentDrawerApi.setState({ placement }); if (placement === 'top') { - // 页面顶部区域的层级只有200，所以设置一个低于200的值，抽屉从顶部滑出来的时候才比较合适 - inContentDrawerApi.setState({ zIndex: 199 }); + inContentDrawerApi.setState({ zIndex: CONTENT_DRAWER_Z_INDEX }); } else { inContentDrawerApi.setState({ zIndex: undefined }); } inContentDrawerApi.open(); }packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (1)
35-37: Consider moving z-index default to shared constantsThe default z-index value could be moved to a shared constants file to maintain consistency across components.
+// In @vben-core/shared/constants +export const DEFAULT_DRAWER_Z_INDEX = 1000; + +// In current file const props = withDefaults(defineProps<Props>(), { appendToMain: false, drawerApi: undefined, - zIndex: 1000, + zIndex: DEFAULT_DRAWER_Z_INDEX, });packages/@core/ui-kit/popup-ui/src/modal/modal.vue (2)
168-170: Consider extracting common functionality to a shared composableThe
appendToMainandzIndexfunctionality is duplicated between modal and drawer components. Consider extracting this into a shared composable for better maintainability.+// New file: @vben-core/composables/use-append-to-main.ts +import { computed, type ComputedRef } from 'vue'; +import { ELEMENT_ID_MAIN_CONTENT } from '@vben-core/shared/constants'; + +export function useAppendToMain(appendToMain: ComputedRef<boolean>) { + return computed(() => appendToMain.value ? `#${ELEMENT_ID_MAIN_CONTENT}` : undefined); +} +// In modal.vue and drawer.vue -const getAppendTo = computed(() => { - return appendToMain.value ? `#${ELEMENT_ID_MAIN_CONTENT}` : undefined; -}); +const getAppendTo = useAppendToMain(appendToMain);Also applies to: 180-180, 198-198
36-37: Maintain consistency with drawer z-index defaultsEnsure the default z-index value is consistent with the drawer component by using the same shared constant.
+import { DEFAULT_DRAWER_Z_INDEX } from '@vben-core/shared/constants'; + const props = withDefaults(defineProps<Props>(), { appendToMain: false, modalApi: undefined, + zIndex: DEFAULT_DRAWER_Z_INDEX, });packages/@core/ui-kit/layout-ui/src/vben-layout.vue (2)
461-462: Consider moving the ID constant declaration closer to related layout variables.The
idMainContentconstant would be better placed with other layout-related variables near the start of the script, improving code organization and maintainability.const contentRef = ref(); +const idMainContent = ELEMENT_ID_MAIN_CONTENT; const { arrivedState,
559-559: Consider documenting the modal/drawer mounting behavior.Since this change enables a new mounting point for modals and drawers, consider:
- Adding comments explaining the mounting behavior
- Updating component documentation to describe when to use
appendToMain- Adding examples demonstrating different mounting scenarios
This will help other developers understand and correctly use this feature.
📜 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 (21)
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(3 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(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/modal/in-content-demo.vue(1 hunks)playground/src/views/examples/modal/index.vue(4 hunks)playground/src/views/examples/vxe-table/basic.vue(3 hunks)
🔇 Additional comments (24)
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogOverlay.vue (1)
10-10: Verify the impact of z-index removal on existing implementations.
The removal of the hardcoded z-index might affect existing modal implementations that rely on the default stacking behavior.
Let's check for any existing usages that might be affected:
packages/effects/common-ui/src/components/page/page.vue (2)
2-9: LGTM! Enhanced type safety with StyleValue import
The addition of the StyleValue type import from Vue improves type safety for style-related computations.
39-48: Verify scroll behavior with modal/drawer integration
The changes improve type safety with StyleValue annotation and enable proper overflow behavior. However, since this component interacts with modal/drawer components through the appendToMain feature, we should verify the scroll behavior in different scenarios.
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogContent.vue (2)
81-91: Template changes look good, verify z-index value
The implementation correctly applies the positioning and z-index to both overlay and content components, maintaining proper stacking context. The animations provide smooth transitions.
Consider verifying if the default z-index of 1000 is appropriate for your application's stacking context:
Also applies to: 96-96
✅ Verification successful
Default z-index value of 1000 is appropriate
The z-index value of 1000 for the Dialog component is consistent with other overlay components in the codebase:
- DialogScrollContent uses z-index: 1000
- SheetContent uses z-index: 1000
- Drawer component uses z-index: 1000
While the layout components use lower z-index values (200 and below) which is correct as dialogs should appear above layout elements:
- Layout header: ~200
- Layout sidebar: 0-1
- Layout footer: varies based on props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other z-index values in the codebase for comparison
rg "z-index|zIndex" --type vue --type css --type scss -A 2
Length of output: 88
Script:
#!/bin/bash
# Search for z-index values using ripgrep with just file extensions
rg "z-index|zIndex" -g "*.vue" -g "*.css" -g "*.scss" -A 2
Length of output: 10773
23-32: Verify prop naming consistency with PR objectives
The PR objectives mention appendToMain but the implemented prop is named appendTo. This might cause confusion for other developers. Consider either:
- Updating the PR description to match the implementation, or
- Renaming the prop to match the intended feature name.
✅ Verification successful
Prop naming is correct, no changes needed
The implementation using appendTo is correct and consistent with the component architecture. The search results show that:
appendTois used consistently in shadcn-ui components (DialogContent.vue and SheetContent.vue) as a flexible prop accepting HTMLElement or stringappendToMainis a different, boolean-type prop used in popup-ui components (modal.ts, drawer.ts) specifically for main content area mounting- The documentation confirms these are two distinct features with different purposes
The PR objectives likely referred to supporting both features, and the implementation correctly uses appendTo for this general-purpose dialog component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of appendToMain in the codebase
rg "appendToMain" --type vue --type ts
Length of output: 66
Script:
#!/bin/bash
# Search for any usage of appendToMain or appendTo in the codebase
rg -t typescript -t javascript -t vue "appendToMain|appendTo"
Length of output: 98
Script:
#!/bin/bash
# Search for any usage of appendToMain or appendTo using ripgrep without file type filters
rg "appendToMain|appendTo"
Length of output: 2610
packages/@core/ui-kit/layout-ui/package.json (1)
43-43: LGTM: Dependency addition aligns with feature requirements
The addition of @vben-core/shared as a workspace dependency is appropriate for accessing shared constants needed for the new mounting features.
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetContent.vue (4)
18-23: LGTM: Well-structured prop interface extensions
The new props appendTo and zIndex are properly typed and align with the PR objectives for flexible mounting and z-index control.
30-33: LGTM: Sensible default values
Default values are appropriate:
appendTo: 'body'maintains backward compatibilityzIndex: 1000follows common UI library conventions
49-59: LGTM: Robust mounting logic implementation
The isAppendToBody function and position computed property effectively handle the mounting behavior:
- Comprehensive checks for body mounting scenarios
- Correct position switching between 'fixed' and 'absolute'
65-71: Verify z-index propagation to nested components
The z-index and position styles are correctly applied to both overlay and content. However, we should verify that z-index values properly cascade to nested dialogs if they exist.
✅ Verification successful
Z-index propagation is properly handled - no nested dialogs found
Based on the codebase analysis, there are no instances of nested DialogContent or SheetContent components. The z-index and position styles are correctly applied at the root level of each dialog/sheet component, and there's no risk of z-index stacking context issues from nesting. The current implementation in SheetContent.vue is safe and follows best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for nested dialog usage patterns
ast-grep --pattern 'DialogContent {
$$$
DialogContent {
$$$
}
$$$
}'
Length of output: 82
Script:
#!/bin/bash
# Search for nested dialog usage in template files
rg -U "(?s)<DialogContent.*?<DialogContent.*?</DialogContent>.*?</DialogContent>" --type vue
# Search for any nested sheet/dialog components
rg -U "(?s)<(Sheet|Dialog)(Content|Root).*?<(Sheet|Dialog)(Content|Root).*?</(Sheet|Dialog)(Content|Root).*?</(Sheet|Dialog)(Content|Root)" --type vue
# Check if there are any components that might use multiple dialogs
rg "<(Sheet|Dialog)(Content|Root)" --type vue -l | xargs -I {} rg -U "(?s)<(Sheet|Dialog)(Content|Root).*<(Sheet|Dialog)(Content|Root)" {} || true
Length of output: 476
Script:
#!/bin/bash
# Search for nested dialog usage in template files without file type restriction
rg -U "(?s)<DialogContent.*?<DialogContent.*?</DialogContent>.*?</DialogContent>"
# Search for any nested sheet/dialog components
rg -U "(?s)<(Sheet|Dialog)(Content|Root).*?<(Sheet|Dialog)(Content|Root).*?</(Sheet|Dialog)(Content|Root).*?</(Sheet|Dialog)(Content|Root)"
# List files with multiple dialog/sheet components
fd -e vue -e js -e jsx -e tsx | xargs rg "<(Sheet|Dialog)(Content|Root)" -l | while read -r file; do
echo "=== $file ==="
rg -U "(?s)<(Sheet|Dialog)(Content|Root).*<(Sheet|Dialog)(Content|Root)" "$file" || true
done
Length of output: 4157
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogScrollContent.vue (2)
17-20: LGTM: Consistent z-index implementation
The zIndex prop implementation matches SheetContent.vue, maintaining consistency across components.
45-45: LGTM: Proper z-index binding
The z-index style binding is correctly implemented on the DialogContent component.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.ts (1)
10-14: LGTM! Well-structured interface additions
The new properties are well-documented and properly typed. The additions of appendToMain, modal, and button visibility controls enhance the component's flexibility.
Also applies to: 72-72, 87-95
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
6-10: LGTM! Consistent implementation with Drawer component
The appendToMain property is well-documented and maintains consistency with the Drawer component implementation.
playground/src/views/examples/modal/index.vue (3)
12-12: LGTM: Modal setup looks good!
The implementation correctly sets up the InContentModal using useVbenModal with proper component connection.
Also applies to: 20-23
49-51: LGTM: Modal handler implementation is correct!
The openInContentModal function properly uses the modal API to trigger the modal.
89-89: LGTM: Template implementation is well structured!
The changes correctly:
- Add auto-content-height to Page component (required for appendToMain)
- Include the modal component
- Add a clear demo section with proper description
Also applies to: 108-111
docs/src/components/common-ui/vben-drawer.md (1)
101-105: LGTM: Clear and comprehensive appendToMain documentation!
The documentation effectively explains:
- The purpose and behavior of appendToMain
- Requirements for proper implementation
- Impact on layout and content
docs/src/components/common-ui/vben-modal.md (1)
112-116: LGTM: Clear and comprehensive appendToMain documentation!
The documentation effectively explains:
- The purpose and behavior of appendToMain
- Requirements for proper implementation
- Impact on layout and content
playground/src/views/examples/drawer/index.vue (1)
20-24: LGTM: Drawer initialization follows existing patterns
The initialization of InContentDrawer follows the same pattern as other drawer instances, maintaining consistency in the codebase.
packages/@core/ui-kit/popup-ui/src/drawer/drawer.vue (1)
119-121: LGTM: Clean implementation of appendToMain functionality
The computed property and its usage in the template are well-implemented. The code is clean and follows Vue best practices.
Also applies to: 130-140
packages/@core/ui-kit/layout-ui/src/vben-layout.vue (2)
14-14: LGTM! Good practice using a shared constant for the content area ID.
Using a shared constant for DOM element IDs ensures consistency across the application and reduces the risk of ID conflicts.
559-559: LGTM! Verify modal/drawer mounting behavior.
The ID binding enables the new appendToMain feature for modals and drawers. This implementation looks correct.
✅ Verification successful
Implementation verified - ID binding works correctly with modal/drawer components
The ID binding is properly implemented and integrated with the modal/drawer mounting system:
- The
ELEMENT_ID_MAIN_CONTENTconstant is defined in shared constants - Both modal and drawer components correctly import and use this ID for the
appendToMainfeature - The components compute the mount target using
getAppendTowhich returns#${ELEMENT_ID_MAIN_CONTENT}whenappendToMainis true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that modal/drawer components reference this ID for mounting
# Search for references to the main content ID in modal/drawer components
rg -l "ELEMENT_ID_MAIN_CONTENT" --type vue --type typescript
# Search for appendToMain prop usage
ast-grep --pattern 'prop: "appendToMain"'
Length of output: 132
Script:
#!/bin/bash
# Let's try different approaches to find the relevant code
# Search for any mentions of appendToMain in the codebase
rg "appendToMain"
# Search for modal or drawer related files
fd -e ts -e vue "(modal|drawer)"
# Search for any ID references that might be related to main content mounting
rg -i "main.*content.*id|content.*main.*id"
Length of output: 3002
Script:
#!/bin/bash
# Let's find the ELEMENT_ID_MAIN_CONTENT constant definition and verify its usage
# Search for the constant definition
rg "ELEMENT_ID_MAIN_CONTENT.*=" -A 1
# Check how it's imported and used in modal/drawer components
rg "ELEMENT_ID_MAIN_CONTENT" -B 2 -A 2
Length of output: 3180
packages/@core/ui-kit/shadcn-ui/src/ui/sheet/SheetOverlay.vue (1)
10-10:
Ensure proper positioning context for absolute positioning
The change from fixed to absolute positioning requires proper positioning context from parent elements. This could lead to positioning issues if parent containers don't establish the correct positioning context.
Consider one of these solutions:
- Add positioning context documentation:
<script setup lang="ts">
import { inject } from 'vue';
import { useScrollLock } from '@vben-core/composables';
+/**
+ * @requires Parent element must establish a positioning context (position: relative|absolute|fixed)
+ */
useScrollLock();- Or revert to
fixedpositioning and handle appendToMain differently:
- <div :data-dismissable-drawer="id" class="bg-overlay absolute inset-0"></div>
+ <div :data-dismissable-drawer="id" class="bg-overlay fixed inset-0"></div>
packages/@core/ui-kit/shadcn-ui/src/ui/dialog/DialogOverlay.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/src/components/common-ui/vben-modal.md (1)
112-116: Fix typo and consider adding a code example.
- There's a typo in the Chinese text: "这招" should be "这种"
- Consider adding a code example to demonstrate the usage of
auto-content-heightwith the Page component.Apply this change:
-打开带这招的弹窗时 +打开带这种的弹窗时Consider adding a code example like:
<Page auto-content-height> <VbenModal appendToMain /> </Page>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/src/components/common-ui/vben-modal.md(2 hunks)
🔇 Additional comments (1)
docs/src/components/common-ui/vben-modal.md (1)
83-83: LGTM! Property additions are well-documented.
The new properties appendToMain and zIndex are clearly documented with appropriate types and default values.
Also applies to: 110-111
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
playground/src/views/examples/drawer/index.vue (3)
20-24: Consider adding English comments for better international collaboration.The component initialization follows good practices, but the comments are in Chinese. Consider translating them to English for better international collaboration.
- // 连接抽离的组件 + // Connect the separated component
47-56: Consider improvements to zIndex handling.The implementation is good, but could be enhanced:
- Extract the magic number 199 as a named constant for better maintainability
- Translate the Chinese comment to English
+const TOP_DRAWER_Z_INDEX = 199; // Z-index for top placement drawer (must be below page header z-index of 200) function openInContentDrawer(placement: DrawerPlacement = 'right') { inContentDrawerApi.setState({ class: '', placement }); if (placement === 'top') { - // 页面顶部区域的层级只有200，所以设置一个低于200的值，抽屉从顶部滑出来的时候才比较合适 - inContentDrawerApi.setState({ zIndex: 199 }); + // Set z-index below 200 for proper animation when drawer slides from top + inContentDrawerApi.setState({ zIndex: TOP_DRAWER_Z_INDEX }); } else { inContentDrawerApi.setState({ zIndex: undefined }); } inContentDrawerApi.open(); }
58-62: Fix typo in comment and translate to English.The implementation is correct, but the comment needs improvement:
- // 这里只是用来演示方便。实际上自己使用的时候可以直接将这些配置卸载Drawer的属性里 + // This is for demonstration purposes only. In practice, these configurations can be directly set as Drawer properties
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
playground/src/views/examples/drawer/index.vue(4 hunks)playground/src/views/examples/vxe-table/basic.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- playground/src/views/examples/vxe-table/basic.vue
🔇 Additional comments (2)
playground/src/views/examples/drawer/index.vue (2)
112-127: LGTM! Well-structured button layout.
The button layout is well-organized with consistent spacing and clear action labels.
130-159: LGTM! Well-implemented in-content drawer demo section.
The implementation follows the established pattern and provides clear user interaction options. The description effectively communicates the purpose of this feature.
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
New Features
Vben DrawerandVben Modalcomponents with new properties:appendToMainandzIndex.InContentDrawerandInContentModalcomponents, enhancing modal and drawer functionalities.DialogContent,SheetContent, andDialogScrollContentcomponents with new properties for improved positioning and z-index control.ModalandDrawerelements to thebasic.vuecomponent for user engagement.Bug Fixes
overflowYin thepage.vuecomponent.Documentation
Chores
@vben-core/shared.