feat: improve page component
#5013
Conversation
|
WalkthroughThe changes include the addition of a new sidebar section for layout components in the documentation, specifically introducing "布局组件" (Layout Components) and a nested item for "Page 页面". A new documentation file for the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
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/effects/common-ui/src/components/page/page.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 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: 0
🧹 Outside diff range and nitpick comments (4)
docs/src/components/introduction.md (1)
9-11: Consider adding English translations for better accessibilitySince this is an open-source project, consider adding English translations alongside the Chinese text for better international accessibility.
## 布局组件 +## Layout Components 布局组件一般在页面内容区域用作顶层容器组件，提供一些统一的布局样式和基本功能。 +Layout components are generally used as top-level container components in page content areas, providing unified layout styles and basic functionality.packages/effects/common-ui/src/components/page/page.vue (3)
44-54: Consider adding type safety and transitionsThe computed
headerStyleimplementation could be improved:
- Add type checking for
preferences.header.mode- Consider adding CSS transitions for smooth header positioning
- Document the z-index value in a constant for better maintainability
+const HEADER_Z_INDEX = 200; + +type HeaderMode = 'fixed' | 'static'; // Add other valid modes + const headerStyle = computed<StyleValue>(() => { return fixedHeader ? { position: 'sticky', - zIndex: 200, + zIndex: HEADER_Z_INDEX, + transition: 'top 0.3s', top: - preferences.header.mode === 'fixed' ? 'var(--vben-header-height)' : 0, + (preferences.header.mode as HeaderMode) === 'fixed' + ? 'var(--vben-header-height)' + : 0, borderBottom: '1px solid hsl(240 5.9% 90%)', } : undefined; });
96-96: Consider adding aria attributes for accessibilitySince this is a fixed header that might affect keyboard navigation, consider adding appropriate ARIA attributes.
- :style="headerStyle" + :style="headerStyle" + :aria-fixed="fixedHeader" + role="banner"
Line range hint
1-128: Consider adding tests for the fixed header functionalityWhile the implementation looks good, tests would help ensure the fixed header behavior works correctly across different scenarios:
- Test header positioning with different preference modes
- Test interaction with scrolling
- Test accessibility features
Would you like me to help create test cases for these scenarios?
📜 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 (7)
docs/.vitepress/config/zh.mts(1 hunks)docs/src/components/introduction.md(1 hunks)docs/src/components/layout-ui/page.md(1 hunks)packages/@core/ui-kit/layout-ui/src/vben-layout.vue(1 hunks)packages/effects/common-ui/package.json(1 hunks)packages/effects/common-ui/src/components/page/page.vue(5 hunks)playground/src/views/examples/modal/index.vue(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- playground/src/views/examples/modal/index.vue
- docs/src/components/layout-ui/page.md
🔇 Additional comments (5)
packages/effects/common-ui/package.json (1)
25-25: Verify the dependency usage scope
The new dependency @vben-core/preferences is correctly added with workspace scope. However, let's verify its usage across the codebase.
✅ Verification successful
Dependency is correctly used and imported
The @vben-core/preferences dependency is properly used in packages/effects/common-ui/src/components/page/page.vue for handling user preferences, which aligns with its intended purpose as documented in the effects README. The dependency is also correctly configured as a workspace package.
- Used in:
packages/effects/common-ui/src/components/page/page.vue - Package location:
packages/@core/preferences - Documentation reference: Found in
packages/effects/README.mdconfirming its use for user preferences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports of @vben-core/preferences
rg "from '@vben-core/preferences'" --type ts --type vue
Length of output: 91
Script:
#!/bin/bash
# Search for any usage of @vben-core/preferences with broader pattern
rg "@vben-core/preferences"
# Also check for specific file extensions since the package might be used in different ways
fd -e ts -e vue -e js . -x rg -l "@vben-core/preferences" {}
Length of output: 1050
packages/effects/common-ui/src/components/page/page.vue (2)
21-22: Documentation looks good
The prop documentation is clear and follows the existing pattern.
95-97: Verify z-index conflicts
The header's z-index of 200 might conflict with other fixed elements. Let's verify the z-index usage across the codebase.
✅ Verification successful
No z-index conflicts detected in the header component
Based on the codebase analysis, there are no z-index conflicts with the header component. The search reveals that most high z-index values are used in overlay components (z-1000+) like dialogs, tooltips, and dropdowns, which are designed to appear above regular content. The header's z-index of 200 sits in a safe range that won't interfere with these overlay components while still maintaining proper layering with regular page content.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for z-index declarations
rg "z-?index:?\s*\d+" --type css --type vue --type scss
Length of output: 85
Script:
#!/bin/bash
# Search for z-index declarations with ripgrep, including Vue files
rg "z-?index:?\s*\d+" --type-add 'vue:*.vue' --type vue --type css --type scss
Length of output: 109
Script:
#!/bin/bash
# Search for z-index declarations using simpler ripgrep command
rg "z-index" -g "*.{vue,css,scss}"
# Also search for Tailwind z-index classes
rg "z-\[?\d+\]?" -g "*.{vue,css,scss}"
Length of output: 11612
docs/.vitepress/config/zh.mts (1)
151-160: LGTM! Documentation structure is well-organized.
The new layout components section is appropriately placed before the common components section, which follows a logical hierarchy as layout components are typically top-level container components.
packages/@core/ui-kit/layout-ui/src/vben-layout.vue (1)
506-506: Verify scrolling behavior with fixed header.
The removal of overflow-hidden allows content to scroll naturally, which is necessary for the fixed header feature. However, please verify:
- No unintended horizontal scrollbars appear
- Content scrolls smoothly under the fixed header
- Layout remains stable during scrolling
✅ Verification successful
The removal of overflow-hidden is safe and appropriate for the fixed header layout.
Based on the codebase analysis:
- The header has its own
overflow-hiddenclass in the wrapper div, which prevents any unwanted header content overflow - The layout uses proper fixed positioning and z-indexing for the header component
- The sidebar has its own
overflow-hiddenmanagement - The content area's scrolling is properly managed through the layout structure and CSS transitions
The removal of overflow-hidden from the flex container is correct as it allows:
- Natural content scrolling under the fixed header
- Proper interaction between the content and fixed elements
- Smooth transitions during layout changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any overflow-related styles that might conflict with this change
rg -A 2 'overflow-[xy]|overflow:\s*hidden' --type css --type vue
Length of output: 94
Script:
#!/bin/bash
# Search for overflow-related styles in all files, including Vue files
rg -A 2 'overflow-[xy]|overflow:\s*hidden'
Length of output: 3628
Script:
#!/bin/bash
# Look for layout-related components and their style definitions
fd -e vue -e css layout
Length of output: 579
Script:
#!/bin/bash
# Check the parent layout components for any scroll-related styles
ast-grep --pattern 'class="$_"' packages/@core/ui-kit/layout-ui/src/vben-layout.vue
Length of output: 85
Script:
#!/bin/bash
# Let's check the content of the main layout components to understand the scroll behavior
cat packages/@core/ui-kit/layout-ui/src/vben-layout.vue packages/@core/ui-kit/layout-ui/src/components/layout-content.vue
Length of output: 15956
Script:
#!/bin/bash
# Check for any scroll-related styles in the layout components
rg -A 2 'scroll|overflow' packages/@core/ui-kit/layout-ui/
Length of output: 3288
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
docs/src/components/layout-ui/page.md (2)
7-13: Consider adding example code to demonstrate the basic structure.While the introduction clearly explains the component's purpose, adding a basic example code snippet would help users understand the component's structure more quickly.
Consider adding something like:
本组件是一个基本布局组件。如果有更多的通用页面布局需求（比如双列布局等），可以根据实际需求自行封装。 +```vue +<template> + <Page title="页面标题" :fixed-header="true"> + <!-- 页面内容 --> + <div>页面内容</div> + + <!-- 使用插槽自定义底部 --> + <template #footer> + <div>页面底部</div> + </template> + </Page> +</template> +```
15-18: Enhance the basic usage section with more details.The current usage section is quite brief. Consider adding:
- Common use cases
- Integration with layout systems
- Best practices for component positioning
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/src/components/layout-ui/page.md(1 hunks)
🔇 Additional comments (1)
docs/src/components/layout-ui/page.md (1)
35-43: LGTM! Slots documentation is complete and clear.
The slots section effectively documents all available slots with clear descriptions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/src/components/layout-ui/page.md (1)
29-29: Enhance thefixedHeaderprop description.The description should clarify that this is specifically for the content area's header, not the app's main header, to avoid confusion.
-| fixedHeader | 固定头部在页面内容区域顶部，在滚动时保持可见 | `boolean` | `false` | +| fixedHeader | 固定头部在页面内容区域顶部，在内容区域滚动时保持可见。注意：这是针对页面组件的头部，而不是应用的主要导航头部 | `boolean` | `false` |playground/src/views/examples/form/basic.vue (1)
342-352: Consider extracting the description text to avoid duplication.The description text is duplicated in both the
descriptionprop and the template content.- description="表单组件基础示例，请注意，该页面用到的参数代码会添加一些简单注释，方便理解，请仔细查看。" fixed-header header-class="pb-0" title="表单组件" > <template #description> <div class="text-muted-foreground"> - <p> - 表单组件基础示例，请注意，该页面用到的参数代码会添加一些简单注释，方便理解，请仔细查看。 - </p> + <p>{{ description }}</p> </div>Add this to the script section:
const description = '表单组件基础示例，请注意，该页面用到的参数代码会添加一些简单注释，方便理解，请仔细查看。';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/src/components/layout-ui/page.md(1 hunks)packages/effects/common-ui/src/components/page/page.vue(6 hunks)playground/src/views/examples/form/basic.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects/common-ui/src/components/page/page.vue
🧰 Additional context used
📓 Learnings (1)
docs/src/components/layout-ui/page.md (2)
Learnt from: mynetfan
PR: vbenjs/vue-vben-admin#5013
File: docs/src/components/layout-ui/page.md:23-24
Timestamp: 2024-12-04T04:43:22.179Z
Learning: 在 `docs/src/components/layout-ui/page.md` 的 `Page` 组件中，`title` 和 `description` 属性有意使用 `string|slot` 类型表示法，表示它们可以接受字符串或插槽。
Learnt from: mynetfan
PR: vbenjs/vue-vben-admin#5013
File: docs/src/components/layout-ui/page.md:31-31
Timestamp: 2024-12-04T04:41:30.161Z
Learning: 在 Vben Admin 项目的 `Page` 组件中，`extra` 内容仅通过 slots 提供，没有对应的 prop 属性。
🔇 Additional comments (4)
docs/src/components/layout-ui/page.md (2)
7-11: LGTM! Clear introduction and guidance.
The introduction effectively explains the component's purpose and provides good guidance about custom layouts.
33-33: LGTM! Important note about header rendering.
The note clearly explains when the header area will not be rendered, which is crucial for users to understand.
playground/src/views/examples/form/basic.vue (2)
338-341: LGTM! Proper usage of the new fixed-header prop.
The fixed-header prop is correctly implemented along with appropriate styling through header-class="pb-0" to adjust padding.
356-364: LGTM! Clean implementation of conditional card rendering.
The v-show directive is appropriately used for toggling between form examples based on the active tab.
mynetfan
changed the title
page component support fixed headerpage component
|
|
|
后续PR已经处理了这个问题，#5033 |
Description
此PR主要修改了
Page组件fixed-header属性用于配置头部区域是否固定。close FEATURE: 建议Page组件header部分支持固定到顶部 #4984Page组件的文档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
Pagecomponent, detailing its structure and usage.fixedHeaderproperty in thePagecomponent for a sticky header feature.Bug Fixes
Documentation
introduction.mdto include information about layout components.Pagecomponent, outlining its usage and properties.