📦 refactor(button): latest shadcn/ui button styling#428
Conversation
mikij
requested review from
Luizgomess,
ribeiromatheuss and
srizzon
and removed request for
Luizgomess and
ribeiromatheuss
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughButton variants and tests were overhauled (new size/type tokens and Testing Library tests), an alert-dialog test expectation was adjusted, and date-picker variants/types were exported with absolute imports and Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/zard/src/lib/shared/components/date-picker/date-picker.component.ts (3)
48-48:⚠️ Potential issue | 🟠 Major
aria-expandedis hardcoded tofalse— accessibility bug.This attribute never reflects the actual popover state. Screen readers will always report the popover as collapsed, even when it's open. Since the component already tracks visibility via
onPopoverVisibilityChange, wire a signal to toggle this properly.♿ Proposed fix
Add a signal to track popover visibility and bind it:
export class ZardDatePickerComponent implements ControlValueAccessor { private readonly datePipe = inject(DatePipe); + private readonly popoverOpen = signal(false); ... - [attr.aria-expanded]="false" + [attr.aria-expanded]="popoverOpen()" ... protected onPopoverVisibilityChange(visible: boolean): void { + this.popoverOpen.set(visible); if (visible) {You'll need to add
signalto the@angular/coreimport.
149-157: 🧹 Nitpick | 🔵 TrivialBare
setTimeoutwithout a delay — consider documenting intent.The
setTimeout(() => { ... })with no delay is a common Angular trick to defer execution to the next macrotask (after the popover DOM renders). It works, but a brief inline comment explaining why it's deferred would help future readers. Minor nit.
38-40:⚠️ Potential issue | 🔴 CriticalAddress TypeScript type mismatch:
ZardDatePickerSizeVariantscannot be assigned to button'sZardButtonSizeVariantsinput.The date-picker's
zSize()is typed asZardDatePickerSizeVariants('sm' | 'default' | 'lg'), but the button's[zSize]input expectsZardButtonSizeVariants('default' | 'xs' | 'sm' | 'lg' | 'icon' | 'icon-xs' | 'icon-sm' | 'icon-lg'). Although the values are a logical subset, TypeScript treats these as distinct types extracted from separate CVA definitions and will reject the assignment. Either cast the value withas ZardButtonSizeVariantsor unify the type definitions to allow direct assignment.
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/button/button.component.spec.ts`:
- Around line 219-222: The test currently asserts Tailwind pseudo-state classes
directly via expect(screen.getByRole('button')).toHaveClass('hover:bg-muted',
'hover:text-foreground'), which can be brittle if compiled output changes;
update the assertions to robustly verify the presence of those literal class
tokens (e.g., assert button.className or button.classList contains
'hover:bg-muted' and 'hover:text-foreground') and do the same for the `link`
type test so the checks rely on token containment rather than exact class
matching.
- Around line 40-68: The test for "does not render loading icon when zLoading is
false" is missing ZardIconComponent in its imports while the other loading tests
include it; update that test's render call so its imports array includes both
ZardButtonComponent and ZardIconComponent (keeping imports consistent across the
describe('loading state') block) to ensure ZardIconComponent is available for
the component under test when checking zLoading behavior.
- Around line 1-2: The spec currently imports and uses Testing Library helpers
(render, screen, waitFor) instead of Angular TestBed/ComponentFixture; refactor
the test to follow the project's TestBed standard by replacing
render/screen/waitFor usage with Angular Testing Utilities: set up
TestBed.configureTestingModule(...) including declarations/providers used by
ButtonComponent, create a ComponentFixture<ButtonComponent> via
TestBed.createComponent(ButtonComponent), call fixture.detectChanges(), and
replace screen queries with fixture.debugElement/nativeElement queries and async
waits with fixture.whenStable() or waitForAsync; update imports to remove
'@testing-library/angular' and '@testing-library/jest-dom' and import TestBed,
ComponentFixture, waitForAsync (or async) from '@angular/core/testing' and the
ButtonComponent symbol.
In `@libs/zard/src/lib/shared/components/button/button.variants.ts`:
- Line 12: The default variant in button.variants.ts currently uses the
`[a]:hover:` prefix which limits the hover style to anchor elements; update the
`default` variant's class string (the `default` key in the variants object) to
replace `[a]:hover:bg-primary/80` with `hover:bg-primary/80` so the hover effect
applies to all button types (`<a z-button>`, `<button z-button>`, `<z-button>`),
matching the other variants like `destructive`, `outline`, `secondary`, `ghost`,
and `link`.
6ad3f15 to
a0a3fc6
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/date-picker/date-picker.component.ts`:
- Around line 94-96: The zSize input (readonly zSize =
input<ZardDatePickerSizeVariants>('default')) currently limits allowed sizes
compared to the wrapped button's ZardButtonTypeVariants, causing an arbitrary
restriction; fix by updating the ZardDatePickerSizeVariants type to include the
full set of button size variants (e.g., add
'xs','icon','icon-xs','icon-sm','icon-lg' to the union) and ensure the input
default stays valid, or alternatively add a concise comment above readonly zSize
explaining why the date picker intentionally restricts sizes and list the
supported variants so consumers understand the constraint; reference
ZardDatePickerSizeVariants and zSize in date-picker.component.ts when making
this change.
libs/zard/src/lib/shared/components/date-picker/date-picker.component.ts
Show resolved
Hide resolved
mikij
force-pushed
the
refactor/button
branch
from
a0a3fc6 to
6614967
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/button/button.variants.ts`:
- Around line 5-8: The buttonVariants string uses "transition-all" which causes
unwanted animations; update the variant definition in buttonVariants (inside the
mergeClasses call) to replace "transition-all" with "transition-colors" so
transitions are limited to color changes and match shadcn/ui behavior. Ensure
the change is made where the class list is composed (the argument to
mergeClasses in button.variants.ts) and keep the rest of the class string
intact.
In `@libs/zard/src/lib/shared/components/date-picker/date-picker.component.ts`:
- Around line 26-31: HEIGHT_BY_SIZE currently types its keys as
Record<NonNullable<ZardDatePickerSizeVariants>, string> but
ZardDatePickerSizeVariants is already NonNullable, so remove the redundant
NonNullable wrapper; change the declaration to use
Record<ZardDatePickerSizeVariants, string> and keep the same entries (xs, sm,
default, lg) so references to HEIGHT_BY_SIZE and the ZardDatePickerSizeVariants
type remain correct.
libs/zard/src/lib/shared/components/date-picker/date-picker.component.ts
Outdated
Show resolved
Hide resolved
mikij
force-pushed
the
refactor/button
branch
from
6614967 to
0f06fb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/button/button.variants.ts (1)
46-51: 🧹 Nitpick | 🔵 Trivial
zLoadingandzDisabledapply identical classes — consider sharing a constant.Both variants resolve to
opacity-50+pointer-events-none(just reordered). If the intent is for them to always match, extracting a sharedconst disabledClasses = 'pointer-events-none opacity-50'reduces drift risk. If they're expected to diverge later, this is fine as-is.♻️ Optional: extract shared disabled/loading classes
+const inactiveClasses = 'pointer-events-none opacity-50'; + export const buttonVariants = cva( ... zLoading: { - true: 'opacity-50 pointer-events-none', + true: inactiveClasses, }, zDisabled: { - true: 'pointer-events-none opacity-50', + true: inactiveClasses, },
🤖 Fix all issues with AI agents
In `@libs/zard/src/lib/shared/components/date-picker/date-picker.component.ts`:
- Around line 26-31: Add a brief explanatory comment above the HEIGHT_BY_SIZE
constant describing that these heights intentionally override the button size
variants (e.g., default: h-9 vs button h-8, lg: h-11 vs button h-9) to
accommodate the date-picker UI, and note that mergeClasses (tailwind-merge) is
used to resolve class conflicts; reference HEIGHT_BY_SIZE,
ZardDatePickerSizeVariants, and mergeClasses so future contributors understand
the deliberate size discrepancy and don’t revert it.
libs/zard/src/lib/shared/components/date-picker/date-picker.component.ts
Show resolved
Hide resolved
mikij
force-pushed
the
refactor/button
branch
from
0f06fb6 to
7725d1f
Compare
There was a problem hiding this comment.
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/button/button.variants.ts (1)
46-51: 🧹 Nitpick | 🔵 TrivialInconsistent class ordering between
zLoadingandzDisabled.Both variants apply the same two classes but in reversed order. While functionally equivalent (Tailwind processes them independently), aligning the order improves readability and signals they're intentionally identical.
Suggested fix
zLoading: { - true: 'opacity-50 pointer-events-none', + true: 'pointer-events-none opacity-50', },
mikij
force-pushed
the
refactor/button
branch
from
7725d1f to
49d9559
Compare
2 participants
What was done? 📝
Refactored button variants to use latest shadcn/ui variants
Screenshots or GIFs 📸
|-----Figma-----|
|-----Implementation-----|
Link to Issue 🔗
closes #425
Type of change 🏗
Breaking change 🚨
visual changes
Checklist 🧐
Summary by CodeRabbit
Style
Documentation
Refactor / Public API
Tests