fix(form-item): The mobile disabled status is not obvious in saas mode #3698
Conversation
WalkthroughAdds a disabled prop to FormItem (mobile-first and pc), computes disabled as formInstance.disabled || props.disabled in renderless, and updates label styling to reflect the disabled state in both Vue components and the theme stylesheet. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User/App
participant FI as FormItem (Vue)
participant RL as Renderless (form-item)
participant UI as Label/UI
U->>FI: Set prop disabled (true/false)
FI->>RL: Initialize with props + formInstance
RL-->>FI: state.disabled = formInstance.disabled || props.disabled
FI->>UI: Render label classes based on state.disabled
alt Disabled
UI-->>U: Label has .is-disabled / disabled text color
else Enabled
UI-->>U: Label uses standard text color
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
![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
🧹 Nitpick comments (4)
packages/theme-saas/src/form-item/index.less (1)
189-191: Align mobile/PC disabled colors for consistencyMobile-first uses
text-color-icon-placeholderon small screens, while SaaS theme appliestext-color-text-disabled. Consider unifying the disabled color or documenting the intentional difference with design.packages/vue/src/form-item/src/pc.vue (1)
288-295: Exposearia-disabledfor a11yAdd
aria-disabledto the label to convey state to assistive tech.Apply:
'label', { class: { [`${classPrefix}form-item__label`]: true, 'is-ellipsis': isMobile && ellipsis, 'is-disabled': state.disabled }, style: state.labelStyle, attrs: { - for: state.labelFor + for: state.labelFor, + 'aria-disabled': state.disabled ? 'true' : null },packages/vue/src/form-item/src/mobile-first.vue (2)
42-44: Disabled label color on mobile: confirm token choiceYou switch to
text-color-icon-placeholder(base) andsm:text-color-text-disabled. If the intent is stronger disabled affordance, consider usingtext-color-text-disabledconsistently, or add a short design note explaining the breakpoint-specific choice.Option:
- state.disabled ? 'text-color-icon-placeholder sm:text-color-text-disabled' : 'text-color-text-secondary', + state.disabled ? 'text-color-text-disabled' : 'text-color-text-secondary',
24-48: Addaria-disabledon the labelExpose the disabled state to AT without changing visuals.
Apply:
<label data-tag="tiny-item-label" v-if="slots.label || label" :class=" m( 'py-3 sm:py-0 sm:min-h-[theme(spacing.7)] relative align-bottom float-left text-sm pr-3 sm:pr-4 box-border leading-5 shrink-0', 'overflow-hidden text-ellipsis', state.labelPosition === 'top' ? 'float-none inline-block text-left sm:text-left leading-none px-0 pt-0 pb-1.5 h-auto min-h-0 sm:py-0 sm:pb-1 sm:min-h-[theme(spacing.0)]' : 'min-h-[theme(spacing.9)]', state.labelPosition === 'right' ? 'text-right sm:text-right' : '', state.labelPosition === 'left' ? 'text-left sm:text-left' : '', state.formInline && state.labelPosition === 'top' ? 'block' : '', state.isDisplayOnly ? 'leading-none h-auto align-[inherit] pr-4' : '', tipContent ? 'pr-5 sm:pr-7' : '', state.labelPosition === 'top' && !state.hideRequiredAsterisk ? 'overflow-visible relative before:absolute before:-left-2.5' : '', state.disabled ? 'text-color-icon-placeholder sm:text-color-text-disabled' : 'text-color-text-secondary', state.formItemSize !== 'mini' ? 'sm:text-sm' : 'sm:text-xs' ) " :style="state.labelStyle" :for="state.labelFor" + :aria-disabled="state.disabled || undefined" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/renderless/src/form-item/vue.ts(1 hunks)packages/theme-saas/src/form-item/index.less(1 hunks)packages/vue/src/form-item/src/mobile-first.vue(2 hunks)packages/vue/src/form-item/src/pc.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (6)
packages/theme-saas/src/form-item/index.less (1)
189-191: Disabled label styling: LGTMThe
.is-disabledhook on__labelmaps cleanly to the class added in pc.vue and applies the correct token.packages/renderless/src/form-item/vue.ts (2)
122-123: Disabled precedence is correct (form OR item)Computing
disabledasform.disabled || props.disabledmatches expected semantics.
122-123: ```shell
#!/bin/bash
set -euo pipefail
echo "---- rg: interface IFormItemProps ----"
rg -n --hidden -S 'interface\s+IFormItemProps\b' || trueecho "---- rg: IFormItemProps occurrences ----"
rg -n --hidden -S '\bIFormItemProps\b' || trueecho "---- rg: 'disabled' occurrences (types & props) ----"
rg -n --hidden -S '\bdisabled\b' || trueecho "---- rg: defineProps or props in packages/renderless ----"
rg -n --hidden -S 'defineProps(|\bprops:\s*{' packages/renderless || trueecho "---- head of packages/renderless/src/form-item/vue.ts (if exists) ----"
if [ -f packages/renderless/src/form-item/vue.ts ]; then
sed -n '1,260p' packages/renderless/src/form-item/vue.ts || true
else
echo "file not found: packages/renderless/src/form-item/vue.ts"
fi</blockquote></details> <details> <summary>packages/vue/src/form-item/src/pc.vue (2)</summary><blockquote> `54-57`: **Public prop added: reflect in TS types and docs** Document `disabled` and ensure it’s present in component typings/examples. --- `138-140`: **Whitespace-only ternary indentation** No functional change. </blockquote></details> <details> <summary>packages/vue/src/form-item/src/mobile-first.vue (1)</summary><blockquote> `192-195`: **Public prop added: reflect in TS types and docs** Mirror the request in renderless/pc: ensure typings and docs include `disabled`. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
PR
fix:移动端禁用状态显示不明显
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Style