-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
perf: modal and drawer api support chain calls #5351
Conversation
|
WalkthroughThis pull request introduces method chaining capabilities to the Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant API as DrawerApi/ModalApi
participant UI as User Interface
Dev->>API: setState({ title: 'New Title' })
Dev->>API: open()
Note over Dev,API: New Approach
Dev->>API: setState({ title: 'New Title' }).open()
API->>UI: Update and Display
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 (
|
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: 1
🧹 Nitpick comments (5)
docs/src/demos/vben-drawer/dynamic/index.vue (1)
16-16
: LGTM! Consider documenting setState chaining.The implementation correctly extends chaining to
setState
, showing good API consistency. Consider updating the documentation to explicitly mention that bothsetData
andsetState
support chaining.docs/src/demos/vben-modal/dynamic/index.vue (1)
16-16
: LGTM! Consider documenting the API symmetry.The implementation maintains perfect symmetry between modal and drawer APIs, which is excellent for developer experience.
Consider documenting the intentional API symmetry between modal and drawer components in the architecture documentation. This pattern makes the API more intuitive and easier to learn.
packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts (1)
144-144
: LGTM! Method chaining support added correctly.The implementation properly returns
this
from bothsetData
andsetState
methods, enabling fluent method chaining while maintaining existing functionality.Consider adding JSDoc comments to document the chainable nature of these methods, for example:
+ /** + * Sets the shared data payload + * @returns this - Returns the instance for method chaining + */ setData<T>(payload: T) {Also applies to: 157-157
packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (1)
154-154
: LGTM! Consistent implementation with DrawerApi.The changes mirror the DrawerApi implementation, maintaining consistency across the codebase.
Consider adding JSDoc comments similar to DrawerApi to maintain documentation consistency:
+ /** + * Sets the shared data payload + * @returns this - Returns the instance for method chaining + */ setData<T>(payload: T) {Also applies to: 167-167
playground/src/views/examples/modal/index.vue (1)
66-71
: LGTM! Effective use of method chaining.The implementation properly demonstrates the new chaining capability while maintaining code readability.
Consider consistent formatting for chained calls. For single-line chains like
handleUpdateTitle
, the current format is good. For multi-line chains, consider this style for better readability:sharedModalApi .setData({ content: '外部传递的数据 content', payload: '外部传递的数据 payload', }) .open();Also applies to: 75-75, 79-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/src/components/common-ui/vben-drawer.md
(1 hunks)docs/src/components/common-ui/vben-modal.md
(1 hunks)docs/src/demos/vben-drawer/dynamic/index.vue
(1 hunks)docs/src/demos/vben-drawer/shared-data/index.vue
(1 hunks)docs/src/demos/vben-modal/dynamic/index.vue
(1 hunks)docs/src/demos/vben-modal/shared-data/index.vue
(1 hunks)packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts
(2 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts
(2 hunks)playground/src/views/examples/drawer/index.vue
(3 hunks)playground/src/views/examples/modal/index.vue
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint (ubuntu-latest)
🔇 Additional comments (6)
docs/src/demos/vben-drawer/shared-data/index.vue (1)
12-17
: LGTM! Verify TypeScript return types.The chaining implementation looks good and matches the PR objectives. The code is more concise than using separate calls.
Let's verify the TypeScript types are properly updated to support chaining:
✅ Verification successful
TypeScript types verified and correctly support chaining
The
setData
method is properly typed withreturn this
enabling method chaining, and uses generic type parameter<T>
for type-safe data passing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DrawerApi class definition and method signatures ast-grep --pattern 'class DrawerApi { $$$ setData($_) { $$$ } $$$ }'Length of output: 82
Script:
#!/bin/bash # Try to find drawer-related type definitions and implementations rg -t typescript -t vue "interface.*Drawer|class.*Drawer|type.*Drawer" -l # Search for setData method definitions rg -t typescript -t vue "setData.*\(" -A 3 # Find drawer-related files fd -e ts -e tsx -e vue drawerLength of output: 830
Script:
#!/bin/bash # Check drawer-api.ts content cat packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts # Backup search for setData implementation rg "setData" packages/@core/ui-kit/popup-ui/src/drawer/drawer-api.ts -A 5Length of output: 3353
docs/src/demos/vben-modal/shared-data/index.vue (1)
12-17
: LGTM! Verify TypeScript return types.The modal implementation maintains consistency with the drawer pattern, showing good API symmetry.
Let's verify the TypeScript types are properly updated:
✅ Verification successful
TypeScript types verified and correct
The
setData
method is properly typed with generic parameter<T>
and returnsthis
for method chaining, matching the drawer implementation pattern. Implementation is verified inpackages/@core/ui-kit/popup-ui/src/modal/modal-api.ts
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ModalApi class definition and method signatures ast-grep --pattern 'class ModalApi { $$$ setData($_) { $$$ } $$$ }'Length of output: 81
Script:
#!/bin/bash # First, find relevant modal type definition files rg -l "type.*Modal|interface.*Modal|class.*Modal" --type=typescript # Then search for setData method signature rg "setData.*\(" --type=typescript -A 2Length of output: 181
Script:
#!/bin/bash # Search in .ts and .tsx files for Modal-related definitions rg -g "*.{ts,tsx}" "type.*Modal|interface.*Modal|class.*Modal" # Search for setData method implementations rg -g "*.{ts,tsx}" "setData.*\(" -A 2Length of output: 2116
playground/src/views/examples/drawer/index.vue (3)
2-2
: LGTM! Good type safety with explicit DrawerState import.The addition of the DrawerState type import improves type safety and code clarity.
49-54
: Well-structured conditional state handling.Good practice of creating a state object and conditionally modifying it before applying. The comment explaining the zIndex logic is particularly helpful.
75-80
: LGTM! Consistent method chaining implementation.The implementation follows the same pattern as the modal examples, maintaining consistency across the codebase.
Also applies to: 84-89
docs/src/components/common-ui/vben-drawer.md (1)
140-140
: LGTM! Method signatures updated to support chainingThe updated return types for
setState
andsetData
methods now correctly reflect the chaining capability, aligning with the PR's objective to support fluent API calls likesetData({...}).open()
.Also applies to: 143-143
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/components/common-ui/vben-modal.md
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
Description
Modal 和 Drawer 的部分API支持链式调用。
以前:
现在:
Type 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
Documentation
modalApi
section todrawerApi
.setState
andsetData
.Improvements
Code Refactoring
this
for fluent interface.