-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add resizable
and ColPage
component
#5188
Conversation
|
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/base/icons/src/lucide.tsOops! 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 WalkthroughThis pull request introduces a comprehensive set of changes focused on enhancing the UI layout and component system. The modifications include adding a new resizable column page component ( Changes
Sequence DiagramsequenceDiagram
participant User
participant ColPage
participant ResizablePanelGroup
participant ResizablePanel
participant ResizableHandle
User->>ColPage: Interact with layout
ColPage->>ResizablePanelGroup: Configure panel group
ResizablePanelGroup->>ResizablePanel: Set panel properties
ResizablePanelGroup->>ResizableHandle: Configure handle
User->>ResizableHandle: Resize panels
ResizableHandle->>ResizablePanelGroup: Update panel sizes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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 (
|
resizable
and ColPage
component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (12)
packages/effects/common-ui/src/components/col-page/index.ts (1)
1-2
: LGTM! Consider documenting component composition.The export structure aligns well with the existing Page component pattern. Since this is a new feature, consider adding a README.md in the col-page directory to document the component's relationship with Page and resizable components.
A README.md would help developers understand:
- Component hierarchy (ColPage -> Page)
- Integration with resizable components
- Usage examples and best practices
packages/@core/ui-kit/shadcn-ui/src/ui/resizable/index.ts (1)
3-3
: Consider pinning radix-vue version.Since we're relying on a specific component (SplitterPanel) from radix-vue, it would be beneficial to ensure version compatibility.
Consider:
- Adding radix-vue version to peerDependencies
- Documenting minimum required version
- Adding version compatibility tests
packages/effects/common-ui/src/components/page/types.ts (1)
1-11
: Documentation and type constraints could be improvedConsider the following improvements:
- Add JSDoc comments for all properties to improve documentation
- Consider adding string literal types for class properties to constrain possible values
- Translate the Chinese comment to English for consistency
export interface PageProps { + /** Title of the page */ title?: string; + /** Description text for the page */ description?: string; + /** CSS class(es) to apply to the content container */ - contentClass?: string; + contentClass?: `${string}`; /** - * 根据content可见高度自适应 + * Automatically adjust height based on visible content */ autoContentHeight?: boolean; + /** CSS class(es) to apply to the header */ - headerClass?: string; + headerClass?: `${string}`; + /** CSS class(es) to apply to the footer */ - footerClass?: string; + footerClass?: `${string}`; }packages/effects/common-ui/src/components/col-page/types.ts (1)
4-8
: Translate Chinese comments to EnglishFor consistency, translate the Chinese comments to English.
/** - * 左侧宽度 + * Left panel width * @default 30 */ /** - * 右侧宽度 + * Right panel width * @default 70 */Also applies to: 14-16
packages/@core/ui-kit/shadcn-ui/src/ui/resizable/ResizablePanelGroup.vue (2)
13-15
: Consider adding explicit return type for props.For better type safety and documentation, consider adding an explicit return type to the props definition:
-const props = defineProps< - { class?: HTMLAttributes['class'] } & SplitterGroupProps ->(); +type ResizablePanelGroupProps = { class?: HTMLAttributes['class'] } & SplitterGroupProps; +const props = defineProps<ResizablePanelGroupProps>();
26-38
: Consider enhancing accessibility attributes.The component could benefit from additional ARIA attributes to improve accessibility:
<SplitterGroup v-bind="forwarded" + role="group" + aria-label="Resizable panel group" :class=" cn( 'flex h-full w-full data-[panel-group-direction=vertical]:flex-col', props.class, ) " > <slot></slot> </SplitterGroup>packages/@core/ui-kit/shadcn-ui/src/ui/resizable/ResizableHandle.vue (3)
34-37
: Consider extracting the long class string into a constant.The inline class string is quite long and could be harder to maintain. Consider extracting it into a constant or utility function for better maintainability.
+const baseClasses = 'bg-border focus-visible:ring-ring relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-offset-1 [&[data-orientation=vertical]>div]:rotate-90 [&[data-orientation=vertical]]:h-px [&[data-orientation=vertical]]:w-full [&[data-orientation=vertical]]:after:left-0 [&[data-orientation=vertical]]:after:h-1 [&[data-orientation=vertical]]:after:w-full [&[data-orientation=vertical]]:after:-translate-y-1/2 [&[data-orientation=vertical]]:after:translate-x-0'; <SplitterResizeHandle v-bind="forwarded" :class=" cn( - 'bg-border focus-visible:ring-ring relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-offset-1 [&[data-orientation=vertical]>div]:rotate-90 [&[data-orientation=vertical]]:h-px [&[data-orientation=vertical]]:w-full [&[data-orientation=vertical]]:after:left-0 [&[data-orientation=vertical]]:after:h-1 [&[data-orientation=vertical]]:after:w-full [&[data-orientation=vertical]]:after:-translate-y-1/2 [&[data-orientation=vertical]]:after:translate-x-0', + baseClasses, props.class, ) "
1-48
: Add component documentation.Consider adding JSDoc comments to document:
- Component purpose and usage
- Props description
- Events emitted
- Example usage
+/** + * ResizableHandle Component + * + * A customizable resize handle component that supports both horizontal and vertical orientations. + * Integrates with radix-vue's SplitterResizeHandle for resize functionality. + * + * @component + * @example + * <ResizableHandle + * withHandle + * class="my-custom-class" + * /> + */ <script setup lang="ts">
40-46
: Consider adding aria-label for accessibility.The handle div should have an aria-label to improve accessibility for screen readers.
<div class="bg-border z-10 flex h-4 w-3 items-center justify-center rounded-sm border" + aria-label="Resize handle" >
packages/effects/common-ui/src/components/col-page/col-page.vue (1)
25-41
: Optimize slot delegation performanceThe slot delegation logic recalculates on every render. Consider memoizing the filtered slot names.
-const delegatedSlots = computed(() => { +const delegatedSlots = computed(() => { + if (!slots) return []; + return Object.keys(slots).filter(key => !['default', 'left'].includes(key)); +}); - const resultSlots: string[] = []; - - for (const key of Object.keys(slots)) { - if (!['default', 'left'].includes(key)) { - resultSlots.push(key); - } - } - return resultSlots; -});playground/src/views/examples/layout/col-page.vue (1)
17-27
: Consider using ref instead of reactive for primitive valuesUsing
reactive
for an object of primitives can lead to unwanted proxy overhead. Consider usingref
for individual values.-const props = reactive({ +const props = { + leftCollapsedWidth: ref(5), + leftCollapsible: ref(true), + leftMaxWidth: ref(50), + leftMinWidth: ref(20), + leftWidth: ref(30), + resizable: ref(true), + rightWidth: ref(70), + splitHandle: ref(false), + splitLine: ref(false), -});playground/src/router/routes/modules/examples.ts (1)
240-250
: Consider grouping layout-related examples under a dedicated section.Since this is introducing a new layout component example, consider restructuring to group future layout-related examples:
{ + name: 'LayoutExamples', + path: '/examples/layout', + meta: { + icon: 'material-symbols:layout-outline', + title: $t('examples.layout.title'), + }, + children: [ { name: 'ColPageDemo', path: '/examples/layout/col-page', component: () => import('#/views/examples/layout/col-page.vue'), meta: { badge: 'Alpha', badgeVariants: 'destructive', icon: 'material-symbols:horizontal-distribute', title: $t('examples.layout.col-page'), }, }, + ], },This structure would:
- Provide better organization for layout-related examples
- Make it easier to add more layout examples in the future
- Maintain consistency with other example categories like forms and tables
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/@core/base/icons/src/lucide.ts
(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/index.ts
(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/resizable/ResizableHandle.vue
(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/resizable/ResizablePanelGroup.vue
(1 hunks)packages/@core/ui-kit/shadcn-ui/src/ui/resizable/index.ts
(1 hunks)packages/effects/common-ui/src/components/col-page/col-page.vue
(1 hunks)packages/effects/common-ui/src/components/col-page/index.ts
(1 hunks)packages/effects/common-ui/src/components/col-page/types.ts
(1 hunks)packages/effects/common-ui/src/components/index.ts
(1 hunks)packages/effects/common-ui/src/components/page/index.ts
(1 hunks)packages/effects/common-ui/src/components/page/page.vue
(2 hunks)packages/effects/common-ui/src/components/page/types.ts
(1 hunks)playground/src/locales/langs/en-US/examples.json
(1 hunks)playground/src/locales/langs/zh-CN/examples.json
(1 hunks)playground/src/router/routes/modules/examples.ts
(1 hunks)playground/src/views/examples/layout/col-page.vue
(1 hunks)
🔇 Additional comments (13)
packages/effects/common-ui/src/components/page/index.ts (1)
1-2
: LGTM! Verify type exports usage.
The export pattern is clean and follows best practices. The addition of type exports supports the new ColPage component integration.
✅ Verification successful
LGTM! Type exports are correctly used by ColPage component
The type exports from page/types.ts
are properly used by the ColPage component, which extends PageProps interface. This is verified by:
- ColPage component imports and extends PageProps from page/types.ts
- The types are correctly used in the implementation
- No other issues found with the type exports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of exported types across the codebase
# Expected: Find references to types from this module in ColPage implementation
# Search for imports from this module
rg -t typescript -t vue "from ['\"].*components/page['\"]"
Length of output: 99
Script:
#!/bin/bash
# Search for imports from page component with ripgrep
# Using more generic pattern without file type flags
rg "from ['\"].*components/page['\"]"
# Also search for potential relative imports
rg "from ['\"]\.\.?/page['\"]"
Length of output: 155
Script:
#!/bin/bash
# Let's check for any files that might be using the Page component or its types
# Search for 'Page' or 'PageProps' usage in the codebase
rg "Page(Props)?" --type-add 'vue:*.vue' --type vue --type ts
# Also check the content of the types file to understand what's being exported
cat packages/effects/common-ui/src/components/page/types.ts
Length of output: 24047
packages/@core/ui-kit/shadcn-ui/src/ui/resizable/index.ts (1)
1-3
: LGTM! Verify radix-vue integration.
The export structure is clean and the component naming is intuitive. The aliasing of SplitterPanel to ResizablePanel provides a more consistent API.
✅ Verification successful
Radix Vue integration verified, but version management needs attention
The integration is correctly set up across multiple packages (@vben/docs, @vben-core/composables, @vben-core/shadcn-ui), but all are using "catalog:" as the version specifier. This appears to be a monorepo-specific version management approach, likely using a version catalog or workspace-level dependency management.
packages/@core/ui-kit/shadcn-ui/src/ui/resizable/index.ts
: The integration is valid and working as intended- Consider documenting the version management strategy in the repository's documentation if not already present
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify radix-vue dependency and integration
# Expected: Find package.json with radix-vue dependency
# Check for radix-vue in package.json files
fd package.json | xargs jq -r 'select(.dependencies["radix-vue"] != null) | "Found radix-vue in \(.name): \(.dependencies["radix-vue"])"'
Length of output: 281
Script:
#!/bin/bash
# Let's check the actual version specified in package.json files
fd package.json | xargs cat | grep -A 2 '"radix-vue":'
# Also check if there are any peer dependencies specified
fd package.json | xargs cat | grep -A 5 '"peerDependencies":'
Length of output: 518
packages/effects/common-ui/src/components/index.ts (1)
3-3
: LGTM!
The new export for col-page is correctly placed and follows the established pattern.
packages/effects/common-ui/src/components/col-page/types.ts (1)
1-26
: Verify width calculations
The default values and width constraints should be verified to ensure they work correctly together.
✅ Verification successful
Let's search for the actual implementation and usage of these props to verify the width calculations.
Width calculations are properly implemented and constrained
The implementation shows that the width calculations are working correctly:
- Default values are properly set (leftWidth: 30, rightWidth: 70)
- The component uses
ResizablePanelGroup
with proper constraints:leftMinWidth
,leftMaxWidth
,leftCollapsedWidth
are properly boundrightMinWidth
,rightMaxWidth
,rightCollapsedWidth
are properly bound
- The playground example demonstrates proper validation:
- Left min/max width constraints are enforced (min: 1%, max: 100%)
- Slider ensures min width can't exceed max width - 1
- Max width can't be less than min width + 1
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ColPageProps to verify width calculations
ast-grep --pattern 'const $props = defineProps<ColPageProps>()' -A 10
Length of output: 71
Script:
#!/bin/bash
# Search for files that import and use ColPageProps
rg -l "ColPageProps" --type ts
# Search for implementation of width calculations
rg "leftWidth|rightWidth" --type ts -A 5
Length of output: 1132
Script:
#!/bin/bash
# Let's look for the actual component implementation to see how these widths are used
fd "col-page" --type f --extension vue --extension tsx --extension jsx
# Also search for any computed styles or width calculations
rg "style.*width" --type vue --type tsx --type jsx -A 5
Length of output: 270
Script:
#!/bin/bash
# Let's examine the component implementation files we found
cat packages/effects/common-ui/src/components/col-page/col-page.vue
cat playground/src/views/examples/layout/col-page.vue
Length of output: 5647
packages/@core/ui-kit/shadcn-ui/src/ui/index.ts (1)
19-19
: LGTM!
The new export for the resizable component follows the established pattern and is properly integrated with the existing exports.
packages/@core/base/icons/src/lucide.ts (1)
31-31
: LGTM!
The GripVertical icon export is appropriately added and follows the established pattern. The icon choice is semantically correct for its intended use in the resizable handle component.
packages/@core/ui-kit/shadcn-ui/src/ui/resizable/ResizablePanelGroup.vue (1)
1-38
: Well-structured component with good TypeScript integration!
The component is well-implemented with proper:
- TypeScript support
- Props forwarding
- Utility class usage
- Slot implementation
playground/src/locales/langs/zh-CN/examples.json (1)
63-64
: LGTM! Translation is accurate and consistent.
The Chinese translation "双列布局" (two-column layout) accurately represents the feature's purpose and maintains consistency with the English version.
packages/@core/ui-kit/shadcn-ui/src/ui/resizable/ResizableHandle.vue (1)
14-19
: LGTM! Props are well-typed and documented.
The TypeScript interface extends SplitterResizeHandleProps
appropriately while adding custom props for flexibility.
playground/src/locales/langs/en-US/examples.json (1)
60-64
: LGTM! Translations are clear and consistent.
The English translations are professional and maintain consistency with the Chinese localization file. Both the "Resize" and "ColPage Layout" entries accurately describe their respective features.
packages/effects/common-ui/src/components/page/page.vue (1)
2-2
: LGTM! Type refactoring improves reusability
Moving from inline Props interface to a separate PageProps type improves type reusability and maintainability.
Let's verify the PageProps type definition:
Also applies to: 20-20
✅ Verification successful
Let me verify the original Props interface to ensure all properties are included in the new PageProps type.
Based on the verification results, I can now generate the final response:
Type refactoring is correctly implemented
The PageProps interface in types.ts contains all necessary properties:
- Optional title, description for page metadata
- Optional styling classes (contentClass, headerClass, footerClass)
- Optional autoContentHeight flag with documentation
The component correctly uses these props through defineProps() with proper default value for autoContentHeight.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PageProps type contains all necessary properties
ast-grep --pattern 'export interface PageProps {
$$$
}'
Length of output: 910
Script:
#!/bin/bash
# Get the original Props interface definition from git history
git log -p packages/effects/common-ui/src/components/page/page.vue | grep -A 20 "interface Props"
Length of output: 0
Script:
#!/bin/bash
# Alternative: Search for Props interface in current file
ast-grep --pattern 'interface Props {
$$$
}' packages/effects/common-ui/src/components/page/page.vue
Length of output: 104
Script:
#!/bin/bash
# Check component usage of props
rg -A 5 "defineProps|withDefaults" packages/effects/common-ui/src/components/page/page.vue
Length of output: 253
packages/effects/common-ui/src/components/col-page/col-page.vue (2)
19-23
: 🛠️ Refactor suggestion
Consider validating width props
The default widths (30/70) add up to 100%, but there's no runtime validation to ensure user-provided values maintain this constraint.
Consider adding a validator:
+const validateWidths = ({ leftWidth, rightWidth }: ColPageProps) => {
+ return leftWidth + rightWidth === 100;
+};
+
const props = withDefaults(defineProps<ColPageProps>(), {
leftWidth: 30,
rightWidth: 70,
resizable: true,
+}, {
+ leftWidth: validateWidths,
+ rightWidth: validateWidths,
});
69-105
: Verify ResizablePanel accessibility
The resizable panels should maintain ARIA attributes for accessibility.
Consider adding ARIA labels to improve accessibility:
<ResizablePanelGroup class="w-full" direction="horizontal">
<ResizablePanel
ref="leftPanelRef"
+ aria-label="Left panel"
:collapsed-size="leftCollapsedWidth"
:collapsible="leftCollapsible"
:default-size="leftWidth"
>
<!-- ... -->
</ResizablePanel>
<ResizablePanel
+ aria-label="Right panel"
:collapsed-size="rightCollapsedWidth"
:collapsible="rightCollapsible"
:default-size="rightWidth"
>
<!-- ... -->
</ResizablePanel>
</ResizablePanelGroup>
Description
resizable
组件Page
组件的基础上扩展一个双列布局组件ColPage
,支持拖拽改变宽度、左侧折叠等功能,以及配套DemoType of change
Please delete options that are not relevant.
pnpm-lock.yaml
unless you introduce a new test example.Checklist
pnpm run docs:dev
command.pnpm test
.feat:
,fix:
,perf:
,docs:
, orchore:
.Summary by CodeRabbit
Release Notes
New Features
GripVertical
icon to the icon set.ResizableHandle
andResizablePanelGroup
components for enhanced layout flexibility.ColPage
component for a customizable dual-column layout.ColPageDemo
added to showcase the dual-column layout in the examples section.Documentation