feat: add confirmDisabled for Dialog
#4959
Conversation
mynetfan
requested review from
anncwb,
vince292007 and
a team
as code owners
|
WalkthroughThe pull request introduces a new property, Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
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/@core/ui-kit/popup-ui/src/modal/modal-api.tsOops! Something went wrong! :( ESLint: 9.15.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 (5)
playground/src/views/examples/modal/auto-height-demo.vue (1)
25-29: Consider adding error handling for robustnessThe async operation could benefit from error handling to ensure the modal states are properly reset even when errors occur.
Consider updating the implementation like this:
function handleUpdate(len: number) { modalApi.setState({ confirmDisabled: true, loading: true }); - setTimeout(() => { - list.value = Array.from({ length: len }, (_v, k) => k + 1); - modalApi.setState({ confirmDisabled: false, loading: false }); - }, 2000); + const DEMO_TIMEOUT = 2000; + setTimeout(() => { + try { + list.value = Array.from({ length: len }, (_v, k) => k + 1); + } catch (error) { + console.error('Failed to update list:', error); + } finally { + modalApi.setState({ confirmDisabled: false, loading: false }); + } + }, DEMO_TIMEOUT); }packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
38-41: Enhance JSDoc documentation for the new property.While the implementation is correct, the documentation could be more comprehensive to match the style of other properties.
Apply this diff to improve the documentation:
/** - * 禁用确认按钮 + * 是否禁用确认按钮。用于控制确认按钮的禁用状态，常用于表单验证等场景 + * @default false */ confirmDisabled?: boolean;packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (1)
44-44: Implementation satisfies dynamic state management requirement.The integration with the existing state management system (
setState,batchStore, and store'sonUpdate) provides robust support for dynamically controlling the confirm button's disabled state, which directly addresses the requirements in issue #4946.Usage example for dynamic updates:
// Single update modalApi.setState({ confirmDisabled: true }); // Batch updates modalApi.batchStore(() => { modalApi.setState({ confirmDisabled: true }); modalApi.setState({ confirmLoading: true }); });docs/src/components/common-ui/vben-modal.md (2)
96-96: Enhance the property description with use cases.The
confirmDisabledproperty documentation could be more helpful by including common use cases. Consider expanding the description to mention scenarios like form validation or data loading states.-| confirmDisabled | 禁用确认按钮 | `boolean` | `false` | +| confirmDisabled | 禁用确认按钮，常用于表单验证、数据加载等场景 | `boolean` | `false` |
Line range hint
1-150: Add usage example for theconfirmDisabledproperty.To help developers understand how to use this new feature effectively, consider adding a demo that showcases:
- Dynamic control of the confirm button's disabled state
- Integration with table selections (as mentioned in issue FEATURE: 能否增加弹框确认按钮的disabled属性，并且能动态修改 #4946)
- Form validation scenarios
This could be added as a new section similar to existing examples like "基础用法" or "自动计算高度".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
docs/src/components/common-ui/vben-modal.md(1 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts(1 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.ts(1 hunks)packages/@core/ui-kit/popup-ui/src/modal/modal.vue(2 hunks)playground/src/views/examples/modal/auto-height-demo.vue(1 hunks)
🔇 Additional comments (6)
playground/src/views/examples/modal/auto-height-demo.vue (1)
25-25: Well-implemented state management for the new confirmDisabled property!
The implementation correctly manages both the confirmDisabled and loading states atomically, preventing user interaction during the update process. This is a good practice that ensures UI consistency.
Also applies to: 28-28
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
38-41: Verify integration with modal state management.
The property is correctly added to ModalProps, but we should verify its integration with the modal system.
✅ Verification successful
Property is properly integrated with modal system
The confirmDisabled property is correctly integrated across the modal system:
- It's initialized in the default state in
modal-api.ts - It's properly bound to the confirm button in the modal template using
:disabled="confirmDisabled" - The state management through
ModalApianduseModalcorrectly handles the property - The template correctly applies the disabled state to the confirm button component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new property is properly integrated across the modal system
# Check if the property is properly handled in related files
echo "Checking modal-related files for proper integration..."
# Look for modal state management files
rg -l "ModalState|ModalApi" --type ts
# Check for proper initialization in default state
ast-grep --pattern 'defaultState = {
$$$
confirmDisabled: $_,
$$$
}'
# Verify the property is used in the template
rg "confirmDisabled|:disabled" --type vue
Length of output: 534
Script:
#!/bin/bash
# Let's check the actual implementation and usage of confirmDisabled
# First, let's see the modal implementation
echo "=== Modal Implementation ==="
rg -A 5 "confirmDisabled" packages/@core/ui-kit/popup-ui/src/modal/modal.ts
# Check use-modal.ts for state management
echo "=== Modal State Management ==="
cat packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts
# Check modal-api.ts for API handling
echo "=== Modal API ==="
cat packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts
# Look for template files with .tsx extension since Vue might be using TSX
echo "=== Template Usage ==="
rg "confirmDisabled|:disabled" --type ts
Length of output: 7241
Script:
#!/bin/bash
# Let's check the modal.vue template to see how the confirmDisabled is used
echo "=== Modal Template ==="
cat packages/@core/ui-kit/popup-ui/src/modal/modal.vue
# Also check for any test files that might verify this functionality
echo "=== Modal Tests ==="
cat "packages/@core/ui-kit/popup-ui/src/modal/__tests__/modal-api.test.ts"
Length of output: 11764
packages/@core/ui-kit/popup-ui/src/modal/modal-api.ts (2)
44-44: LGTM! The implementation aligns with PR objectives.
The addition of confirmDisabled with a default value of false is well-integrated into the existing state management pattern and maintains backward compatibility.
44-44: Verify type definitions for ModalState.
Please ensure that the confirmDisabled property is properly defined in the ModalState type interface.
✅ Verification successful
Let me search for ModalProps definition since ModalState extends it, and the confirmDisabled property might be defined there.
Type definition for confirmDisabled is properly defined
The confirmDisabled property is correctly defined in the ModalProps interface (line 41), which is extended by ModalState. The property is defined as an optional boolean with proper documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ModalState type definition includes confirmDisabled property
# Expected: Find type definition with confirmDisabled property
# Search for ModalState type definition
ast-grep --pattern 'interface ModalState {
$$$
confirmDisabled$_
$$$
}'
# Alternatively, search using ripgrep
rg -A 20 'interface\s+ModalState'
Length of output: 1592
Script:
#!/bin/bash
# Search for ModalProps interface definition
ast-grep --pattern 'interface ModalProps {
$$$
}'
# Also search with ripgrep for better context
rg -A 20 'interface\s+ModalProps'
Length of output: 8815
packages/@core/ui-kit/popup-ui/src/modal/modal.vue (2)
62-62: LGTM! Property addition aligns with existing patterns
The confirmDisabled property is correctly integrated into the component's prop system using the usePriorityValues utility, maintaining consistency with other modal control properties.
289-289: LGTM! Verify interaction between disabled and loading states
The disabled binding is correctly implemented. However, it's good practice to verify the interaction between the disabled and loading states.
Let's verify the component's usage patterns:
✅ Verification successful
Based on the search results, I can see that the modal component is used in various scenarios, and there's a clear example in playground/src/views/examples/modal/auto-height-demo.vue where both confirmDisabled and loading states are used together correctly. Let me verify the implementation further with one more search:
Let me try one more search to verify the implementation:
Implementation verified through usage patterns
Based on the search results, particularly from playground/src/views/examples/modal/auto-height-demo.vue, we can see that both states (confirmDisabled and loading) are used together correctly in real implementations. The example shows proper state management where both states are toggled simultaneously:
modalApi.setState({ confirmDisabled: true, loading: true });
// ... async operation ...
modalApi.setState({ confirmDisabled: false, loading: false });This demonstrates that the disabled binding works well alongside the loading state, and the implementation is being used as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of the modal component to understand common patterns
# and ensure the new disabled state works well with existing implementations
# Look for modal usage patterns
rg -A 5 "modalApi\.setState|confirmLoading.*true"
# Look for potential edge cases in modal state management
ast-grep --pattern 'modalApi?.setState((prev) => { $$$
return { $$$, confirmLoading: true, $$$ };
})'
Length of output: 5755
Script:
#!/bin/bash
# Look for the modal component's implementation to verify the disabled and loading state handling
rg -A 10 "confirmDisabled.*loading" packages/@core/ui-kit/popup-ui/src/modal/
Length of output: 79
Script:
#!/bin/bash
# Look for the modal component's implementation focusing on the button props
rg -A 5 ":disabled.*:loading|:loading.*:disabled" packages/@core/ui-kit/popup-ui/src/modal/
Length of output: 93
Description
为
Dialog添加confirmDisabled属性用于控制提交按钮的禁用状态。在一些类似表单弹窗的场景下，可以在不满足提交条件时自行禁用提交按钮。close #4946
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
confirmDisabledfor the modal, allowing users to disable the confirm button.Documentation
confirmDisabledproperty and its functionality.