🐛 fix(checkbox): adding disabled state for reactive forms usage#407
🐛 fix(checkbox): adding disabled state for reactive forms usage#407Luizgomess merged 4 commits intomasterfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Badge types & component libs/zard/src/lib/shared/components/badge/badge.variants.ts, libs/zard/src/lib/shared/components/badge/badge.component.ts, libs/zard/src/lib/shared/components/badge/demo/default.ts |
Split ZardBadgeVariants into ZardBadgeTypeVariants and ZardBadgeShapeVariants; removed rounded-md base; added h-5 to some zType variants; retyped component inputs; demo sets zShape="pill" on second badge. |
Checkbox core & tests libs/zard/src/lib/shared/components/checkbox/checkbox.component.ts, libs/zard/src/lib/shared/components/checkbox/checkbox.component.spec.ts |
Rewrote component to Signals (checked signal, disabledByForm, computed disabled); added zDisabled (booleanAttribute) and implemented setDisabledState(); removed ChangeDetectorRef; updated template/host bindings, ARIA, and tests (ngModel/test assertions updated). |
Checkbox variants & tokens libs/zard/src/lib/shared/components/checkbox/checkbox.variants.ts |
Replaced literal size classes with semantic tokens (size-4, size-6); adjusted focus-ring tokens; exported granular variant types (ZardCheckboxShape/Size/Type). |
Checkbox demos libs/zard/src/lib/shared/components/checkbox/demo/* |
Removed standalone: true; added changeDetection: ChangeDetectionStrategy.OnPush; disabled.ts uses a reactive FormGroup disabled in ngOnInit; shape.ts fixes bindings/typo and renames model fields. |
Checkbox docs & event plugin libs/zard/src/lib/shared/components/checkbox/doc/api.md, libs/zard/src/lib/shared/core/provider/event-manager-plugins/zard-event-manager-plugin.ts |
Docs: added zDisabled to API. Event plugin: moved handler(event) inside shouldApplyModifier so handler runs only when modifier applies. |
Avatar component & docs/tests libs/zard/src/lib/shared/components/avatar/avatar.component.ts, libs/zard/src/lib/shared/components/avatar/doc/api.md, libs/zard/src/lib/shared/components/avatar/avatar.component.spec.ts |
Added NgOptimizedImage; zSrc now accepts `string |
Sequence Diagram(s)
sequenceDiagram
autonumber
actor User as User
participant FG as FormGroup
participant AF as AngularForms
participant CB as ZardCheckboxComponent
User->>FG: call disable()
FG->>AF: mark control disabled
AF->>CB: setDisabledState(true)
CB->>CB: disabledByForm.set(true)
CB->>CB: computed_disabled = zDisabled || disabledByForm
CB->>CB: update aria-disabled & CSS classes
User->>CB: click
alt computed_disabled true
CB-->>User: ignore click (no toggle)
else computed_disabled false
CB->>CB: onCheckboxChange() toggle checked signal
CB->>AF: invoke registered onChange
CB->>CB: emit checkChange
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- ✨ feat(core): event manager plugins with CLI update #348 — modifies the same event-manager plugin area; likely related to the moved handler invocation.
Suggested reviewers
- ribeiromatheuss
- Luizgomess
- srizzon
Poem
Badges slim their types and tidy seams,
Checkboxes now hum with Signal dreams,
Forms can silence them with one small call,
OnPush keeps renders lean and small,
Handlers wake only when modifiers fall. 🦊
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately describes the main change: adding disabled state support for checkbox in reactive forms, which is the primary objective of this PR. |
| Description check | ✅ Passed | The description covers key sections: what was done, breaking changes, type of change selection, and testing checklist. However, screenshots/GIFs placeholders are unfilled and Safari/Firefox testing incomplete. |
| Linked Issues check | ✅ Passed | The PR successfully implements all coding requirements from #406: setDisabledState() method, WritableSignal for disabled state, computed signal combining states, template bindings for disabled styling/aria, interaction handler guards, and removal of manual workarounds. |
| Out of Scope Changes check | ✅ Passed | Changes to badge, avatar, and event manager are minor refactoring outside the primary scope. Badge changes are acknowledged but minimal. Avatar and event manager changes appear to be technical debt improvements rather than scope creep. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/zard/src/lib/shared/core/provider/event-manager-plugins/zard-event-manager-plugin.ts (1)
44-67: Document the aria-disabled behavior in JSDoc — implementation is correct.The handler invocation change is intentional and correct for CVA disabled state support. When
aria-disabled="true", handlers are properly suppressed, aligning with reactive forms patterns.Update the JSDoc (lines 4-25) to document the disabled-element behavior. Add a note that handlers are skipped when
aria-disabled="true":/** * Angular EventManagerPlugin that provides event modifier syntax for templates. * * Supports modifiers: .prevent, .stop, .stop-immediate, .prevent-with-stop * Supports key filters: enter, escape, {enter,space} * * Note: Handlers are suppressed when the element has aria-disabled="true". * This enables proper disabled state handling in form controls. * * `@example` * Prevent default on any click * (click.prevent)="handler()" * ... */Current usage is minimal (single instance in codebase), reducing breaking change risk for internal consumers. For external package users, document this behavior change in the changelog.
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/checkbox/checkbox.component.ts`:
- Around line 98-104: The component's plain boolean checked field doesn't
trigger OnPush change detection when writeValue() updates it; convert checked
into a Signal (e.g., checked = signal(false)) and update writeValue(val:
boolean) to set the signal (checked.set(val)); update all template bindings that
referenced checked to call the signal (checked()) including the [checked]
binding and the opacity binding so they reflect form-driven changes; ensure
imports/initialization for createSignal/signal and adjust any other internal
references to use checked() or checked.set(...) as appropriate.
- Around line 35-56: Remove host-level focus management and restore native input
focusability: delete tabindex="-1" from the <input> element and remove any
tabindex="0", role="checkbox", aria-checked or aria-disabled attributes added to
the host wrapper. Rely on the native <input type="checkbox"> for focus, keyboard
handling and disabled behavior (keep [checked]="checked",
[disabled]="disabled()", [id]="z.id()", [class]="classes()"), and leave the
<label [for]="z.id()" [class]="labelClasses()"> as-is; this applies where
similar host tabindex/ARIA were added (see the other block around lines 67-74).
In `@libs/zard/src/lib/shared/components/checkbox/doc/api.md`:
- Around line 9-14: Fix the typos and code-span spacing in the API table:
correct "chekbox" to "checkbox" for the Description of zType, zSize, zShape;
normalize the Type column code spans to remove extra spaces and backslash
escapes so they read e.g. `default | destructive`, `default | lg`, and `default
| circle | square`; ensure default values remain in backticks (e.g. `default`,
`false`) and update the rows for zType, zSize, zShape, zDisabled accordingly.
There was a problem hiding this comment.
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 (2)
libs/zard/src/lib/shared/components/checkbox/checkbox.component.spec.ts (2)
41-64: Redundant[zDisabled]binding with Reactive Forms.The
[zDisabled]="form.get('privacyCheckbox')?.disabled"binding on line 47 is redundant when using Reactive Forms. TheformControlNamedirective already callssetDisabledState()on the ControlValueAccessor when the control is enabled/disabled programmatically. This dual binding may cause confusion and the template binding won't update reactively after initial render sinceform.get()?.disabledisn't a signal.Consider removing the
[zDisabled]binding and relying solely on the Reactive Forms integration:Suggested fix
- <span z-checkbox formControlName="privacyCheckbox" [zDisabled]="form.get('privacyCheckbox')?.disabled"> + <span z-checkbox formControlName="privacyCheckbox"> Accept Privacy Policy </span>
201-208: Use separatetoHaveClassassertions to match codebase patterns.Split the class assertions into individual calls to align with the established pattern throughout the codebase (e.g., menu-label and menu-shortcut components). Each assertion on its own line is easier to read and debug:
expect(disabledCheckbox).toHaveClass('disabled:cursor-not-allowed'); expect(disabledCheckbox).toHaveClass('disabled:opacity-50');
♻️ Duplicate comments (2)
libs/zard/src/lib/shared/components/checkbox/checkbox.component.ts (2)
69-76: Host-level focus management deviates from WAI-ARIA best practices.The current approach removes the native
<input>from tab order (tabindex="-1") while making the host focusable (tabindex='0'). Per WAI-ARIA guidance, native<input type="checkbox">provides built-in semantics, keyboard handling, and proper disabled behavior. The host lacksrole="checkbox"andaria-checkedattributes needed for custom checkbox semantics.
100-105:checkedshould be a signal for proper OnPush reactivity.With
ChangeDetectionStrategy.OnPush, the plaincheckedfield won't trigger template updates when modified inwriteValue()oronCheckboxChange(). WhilemarkForCheck()is called inwriteValue, converting to a signal would align with the component's existing signal-based architecture and ensure consistent reactivity.
mikij
force-pushed
the
fix/#406-checkbox
branch
from
e233bc2 to
c184f30
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/zard/src/lib/shared/components/checkbox/checkbox.component.spec.ts (1)
41-64: The[zDisabled]binding defeats the purpose ofsetDisabledState.The binding
[zDisabled]="form.get('privacyCheckbox')?.disabled"manually syncs the disabled state, butsetDisabledStatefrom the CVA should handle this automatically when the form control is disabled. This workaround:
- Doesn't actually test that
setDisabledStateworks correctly- Creates potential for desync between the form control's state and the visual state
- Re-introduces the pattern the PR aims to eliminate
Remove the
[zDisabled]binding to let the CVA'ssetDisabledStatehandle the form-driven disabled state:🛠️ Suggested fix
`@Component`({ imports: [ZardCheckboxComponent, ReactiveFormsModule], template: ` <form [formGroup]="form"> <span z-checkbox formControlName="termsCheckbox">Agree to Terms</span> <span z-checkbox formControlName="newsletterCheckbox">Subscribe to Newsletter</span> - <span z-checkbox formControlName="privacyCheckbox" [zDisabled]="form.get('privacyCheckbox')?.disabled"> + <span z-checkbox formControlName="privacyCheckbox"> Accept Privacy Policy </span> </form> `, })
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/checkbox/checkbox.component.spec.ts`:
- Line 276: Remove or resolve the commented-out assertion for
termsCheckbox.checked: either delete the commented line or re-enable and make
the test reliably assert DOM state by ensuring you trigger change detection and
events (e.g., use checkbox.nativeElement.click() or dispatch a 'change' event,
then call fixture.detectChanges() and await fixture.whenStable() if async)
before expecting termsCheckbox.checked toBeTruthy(); if this is a known
limitation, replace the commented line with a short comment explaining why the
DOM can't reflect the value and reference the termsCheckbox variable and
surrounding test setup in checkbox.component.spec.ts.
In `@libs/zard/src/lib/shared/components/checkbox/doc/api.md`:
- Line 12: The markdown table row for the zSize prop has extra spaces inside the
code span for the type (currently shows "`default \| lg `"); update the code
span for zSize's type to remove the leading and trailing spaces so it reads
"`default | lg`" (edit the table cell containing `zSize` and the type column to
fix the spacing and resolve MD038).
libs/zard/src/lib/shared/components/checkbox/checkbox.component.spec.ts
Outdated
Show resolved
Hide resolved
mikij
force-pushed
the
fix/#406-checkbox
branch
from
c184f30 to
22f67dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/checkbox/checkbox.component.spec.ts`:
- Around line 47-49: Remove the redundant manual disabled binding in the test
template: delete the [zDisabled]="form.get('privacyCheckbox')?.disabled" from
the z-checkbox span so the component's disabled state is driven solely by the
reactive FormControl via formControlName; ensure the test disables the control
using the FormControl initial state ({ disabled: true }) or control.disable()
and assert the component's disabled state via the CVA setDisabledState path
(e.g., checking the CheckboxComponent instance or its computed disabled() after
fixture.detectChanges()) to validate CVA integration.
In `@libs/zard/src/lib/shared/components/checkbox/checkbox.component.ts`:
- Line 17: The import currently pulls noopFn from the merge-classes module which
couples an unrelated utility to class merging; extract noopFn into its own
module (e.g., "@/shared/utils/noop" or "@/shared/utils/functions") and update
usages. Create a new export for noopFn (named noopFn) in the new module, remove
its export from merge-classes, and update the import in CheckboxComponent
(replace import { mergeClasses, noopFn } from '@/shared/utils/merge-classes'
with separate imports: mergeClasses from its module and noopFn from the new
module) as well as any other files importing noopFn from merge-classes.
♻️ Duplicate comments (1)
libs/zard/src/lib/shared/components/checkbox/doc/api.md (1)
12-12: Fix trailing space in code span forzSizetype.The code span has a trailing space before the closing backtick:
`default \| lg `. This triggers markdownlint MD038.✏️ Suggested fix
-| `zSize` | checkbox size | `default \| lg ` | `default` | +| `zSize` | checkbox size | `default \| lg` | `default` |
libs/zard/src/lib/shared/components/checkbox/checkbox.component.spec.ts
Outdated
Show resolved
Hide resolved
mikij
force-pushed
the
fix/#406-checkbox
branch
from
22f67dc to
3ebec3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/avatar/avatar.component.ts`:
- Line 3: The zSrc input currently uses the overly broad SafeValue type; change
the import to SafeUrl and update the zSrc Input's type to SafeUrl (or SafeUrl |
string | null as appropriate) so the avatar component uses a URL-safe type;
locate the AvatarComponent (avatar.component.ts) and replace any SafeValue
references with SafeUrl in both the import and the zSrc Input declaration to
enforce stricter type safety for [ngSrc] bindings.
- Around line 13-22: The NgOptimizedImage <img> using fill in avatar.component
should include a sizes attribute to enable optimized srcset generation; update
the <img> built by zSrc()/imageError() to add sizes="100%" (or an appropriate
responsive value) when using fill, or if you manage a custom ngSrcset add
disableOptimizedSrcset to the <img> instead; also consider adding priority on
the <img> if this avatar is above-the-fold.
8c6f060 to
9fdbccc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/avatar/doc/api.md`:
- Line 13: Update the API docs to reflect the actual input type for the avatar
component: change the `zSrc` entry to show `string | SafeUrl` (the same union
typed on the zSrc input in avatar.component.ts) so the documentation matches the
component signature; locate the `zSrc` row in
libs/zard/src/lib/shared/components/avatar/doc/api.md and replace the shown type
`string` with `string | SafeUrl`.
fe95dfd to
4dc1191
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/avatar/avatar.component.ts`:
- Around line 23-37: The avatar component fails to recover when zSrc changes
because imageError/imageLoaded remain stuck; add a Solid effect watching zSrc()
that resets the image state signals (clear imageError and imageLoaded) whenever
zSrc() changes so the `@if` branches can re-render and new images can load; place
this effect near the existing image handlers (onImageError, onImageLoad,
imageLoaded, imageError, zSrc) so it runs on source changes and restores normal
loading/fallback behavior.
4dc1191 to
02bd508
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/avatar/avatar.component.spec.ts`:
- Around line 145-161: The test should avoid casting avatarComponent to any and
instead drive public behavior by triggering native image events: after setting
hostComponent.zSrc and fixture.detectChanges(), locate the rendered <img>
element (query from fixture.nativeElement or DebugElement), dispatch a new
'load' or 'error' Event to simulate success/failure, call
fixture.detectChanges() again, and then assert the avatarComponent's public
observable/signal state via its public API; replace direct calls to
(avatarComponent as any).imageLoaded.set(...) and imageError.set(...) with
dispatchEvent(new Event('load')) or dispatchEvent(new Event('error')) on the img
to reset imageLoaded and imageError when hostComponent.zSrc changes.
98b4a8a to
d296fee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/avatar/avatar.component.spec.ts`:
- Around line 146-164: Extend the existing test that changes hostComponent.zSrc
to also exercise the error path: after setting hostComponent.zSrc =
'first-image.jpg' and fixture.detectChanges(), locate the img and dispatch an
'error' event (imgElement.dispatchEvent(new Event('error'))) to set
AvatarComponent.imageError = true and fixture.detectChanges(); then change
hostComponent.zSrc = 'second-image.jpg' and fixture.detectChanges() and assert
the component reset state by checking the img element exists, its src contains
'second-image.jpg', and that the component properties AvatarComponent.imageError
and AvatarComponent.imageLoaded are reset (false) to verify error-path reset
behavior.
libs/zard/src/lib/shared/components/avatar/avatar.component.spec.ts
Outdated
Show resolved
Hide resolved
mikij
force-pushed
the
fix/#406-checkbox
branch
from
d296fee to
1443e21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/avatar/avatar.component.ts`:
- Around line 122-133: The two internal state signals imageError and imageLoaded
on ZardAvatarComponent are currently public; if they are not intended as part of
the component's public API, change their visibility to protected to encapsulate
internal state. Locate the readonly signal declarations named imageError and
imageLoaded in class ZardAvatarComponent and update their access modifiers from
public (implicit) to protected so consumers cannot access them directly while
keeping their readonly and signal behavior intact.
1443e21 to
4290962
Compare
mikij
force-pushed
the
fix/#406-checkbox
branch
from
4290962 to
759bacd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/avatar/avatar.component.ts`:
- Line 54: Replace the noncanonical fractional Tailwind spacing in the avatar
badge class: locate the class string containing "absolute -right-1.25
-bottom-1.25 z-20 h-5 w-5 text-green-500" in avatar.component (the element that
renders the status badge) and change the spacing utilities to the canonical
multipliers "-right-5 -bottom-5" so the classname becomes "absolute -right-5
-bottom-5 z-20 h-5 w-5 text-green-500".
2 participants
What was done? 📝
Added disabled state for checkbox when it is used in reactive forms. Refactored code to follow our latest standards. Small refactoring for badge added.
Screenshots or GIFs 📸
|-----Figma-----|
|-----Implementation-----|
Link to Issue 🔗
closes #406
Type of change 🏗
Breaking change 🚨
zDisabled input instead of disabled
Checklist 🧐
Summary by CodeRabbit
Bug Fixes
New Features
Style
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.