-
Notifications
You must be signed in to change notification settings - Fork 122
feat(sender): add sendDisabled prop #458
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
Conversation
✅ Deploy Preview for antd-design-x-vue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdded an optional boolean prop Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sender/Sender.vue (1)
38-68: Fix inverted boolean semantics forSendDisabledinonSendDisabledCurrently the disabled state is computed as:
const { // ... onSubmit, SendDisabled, // ... } = defineProps<SenderProps>(); // ... const actionsButtonContextProps = computed(() => ({ prefixCls: actionBtnCls.value, onSend: triggerSend, onSendDisabled: SendDisabled ? !SendDisabled() : !innerValue.value, // ... }));Given the PR description (“custom send button disabled state”) and the prop name
SendDisabled?: () => boolean, callers will naturally implementSendDisabled()to returntruewhen the button should be disabled. The current!SendDisabled()inverts that:
SendDisabled()returns true →onSendDisabledbecomes false → send button effectively enabled.SendDisabled()returns false →onSendDisabledbecomes true → send button disabled.This is almost certainly backwards for a “Disabled” API.
You can fix the semantics while preserving the fallback by using the function value directly:
const actionsButtonContextProps = computed(() => ({ prefixCls: actionBtnCls.value, onSend: triggerSend, - onSendDisabled: SendDisabled ? !SendDisabled() : !innerValue.value, + // If provided, SendDisabled() should return the actual disabled state. + onSendDisabled: SendDisabled ? SendDisabled() : !innerValue.value, onClear: triggerClear, onClearDisabled: !innerValue.value, onCancel, onCancelDisabled: !loading, onSpeech: () => triggerSpeech(false), onSpeechDisabled: !speechPermission.value, speechRecording: speechRecording.value, disabled, }));(Separately, see the comment in
src/sender/interface.tsabout renaming this prop to camelCasesendDisabledfor consistency and template usability.)Also applies to: 281-293
🧹 Nitpick comments (1)
src/sender/interface.ts (1)
44-90: Consider renamingSendDisabledprop to camelCasesendDisabled
SenderPropsuses camelCase for all props (submitType,allowSpeech,autoSize, etc.), but this new prop is PascalCase:SendDisabled?: () => boolean;Two concerns:
- API consistency: A PascalCase prop stands out against all other camelCase props and is easy to misuse.
- Template binding: In Vue templates,
send-disablednormalizes tosendDisabled, notSendDisabled, so template users will likely be unable to bind this prop without TSX/JSX, which defeats the purpose of a public API.Before release, I’d strongly suggest normalizing this:
-export interface SenderProps { +export interface SenderProps { @@ - disabled?: boolean; - SendDisabled?: () => boolean; + disabled?: boolean; + sendDisabled?: () => boolean;…and updating the corresponding usage in
Sender.vue(destructuring andactionsButtonContextProps) tosendDisabledas well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sender/Sender.vue(2 hunks)src/sender/interface.ts(1 hunks)src/sender/useSpeech.ts(1 hunks)
🔇 Additional comments (1)
src/sender/useSpeech.ts (1)
148-150: Formatting-only change; behavior unchangedThe added trailing newline after the return has no impact on runtime behavior. The hook’s logic and API remain intact.
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sender/Sender.vue(3 hunks)src/sender/interface.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sender/interface.ts
🧰 Additional context used
🪛 GitHub Actions: ci
src/sender/Sender.vue
[error] 175-175: TS2322: Type '(message: string) => void' is not assignable to type 'boolean'.
🪛 GitHub Check: ci
src/sender/Sender.vue
[failure] 175-175:
Type '(message: string) => void' is not assignable to type 'boolean'.
🔇 Additional comments (1)
src/sender/Sender.vue (1)
51-51: LGTM! Prop declaration is correct.The
sendDisabledprop is properly declared and will allow external control of the send button's disabled state.
wzc520pyfm
left a comment
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.
Great!
Custom send button disabled for sending
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.