feat(widgets): add hover and press animation to Button component#1761
feat(widgets): add hover and press animation to Button component#1761pixeltannu wants to merge 3 commits into
Conversation
- Add startHover/endHover/startPress methods with progress-based transitions - Hover background color and press color-invert effect, animationMs configurable - Auto-detect focus changes in _renderSelf since isFocused is set directly - Respects prefersReducedMotion - Add tests for hover/press behavior Closes Karanjot786#1735
github-actions
Bot
added
type:feature
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesButton hover/press animation
Vitest dependency version pinning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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: 3
🧹 Nitpick comments (1)
packages/widgets/src/input/Button.ts (1)
260-264: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsolidate animation scheduling to avoid duplication.
This setTimeout pattern duplicates
_scheduleAnimation(). Consider reusing the existing method.♻️ Suggested refactor
- if (isAnimating && this._animationTimer == null) { - this._animationTimer = setTimeout(() => { - this._animationTimer = null; - this.markDirty(); - }, 16); - } + if (isAnimating) { + this._scheduleAnimation(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/widgets/src/input/Button.ts` around lines 260 - 264, The animation timer setup logic in the conditional block checking isAnimating and _animationTimer is duplicating the functionality already present in the _scheduleAnimation() method. Replace the inline setTimeout code that sets _animationTimer, nullifies it after delay, and calls markDirty() with a direct call to _scheduleAnimation() instead to consolidate the animation scheduling logic into a single reusable method.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/widgets/src/input/Button.test.ts`:
- Around line 199-206: The assertion in the test case for focusing the button
(where isFocused = true) is too weak. Replace the
`expect(screen.back[1][1].bg).toBeDefined()` assertion with a stronger check
that verifies the actual hover background color value is applied correctly when
the button is focused. Instead of just checking if bg is defined, assert that bg
contains the expected hover background color value (not just any truthy value
like { type: 'none' }).
- Around line 208-212: The test `'startPress triggers re-render and does not
throw'` has a misleading name because it only verifies that `startPress()` does
not throw an exception, but does not actually verify that a re-render is
triggered. Either rename the test to accurately reflect what is being tested
(e.g., `'startPress does not throw'`), or add meaningful assertions that verify
the re-render behavior by checking that the press animation schedules a
re-render, following the same pattern used in the `startHover` test.
- Around line 179-197: The tests for `startHover()` and `endHover()` are failing
because `prefersReducedMotion()` returns true in the test environment, causing
both methods to return early before calling `markDirty()`. Fix this by mocking
`caps.motion` to true in both test cases (for 'startHover schedules a re-render
via markDirty after the animation tick' and 'endHover clears hover state and
calls markDirty') to enable animations and allow the methods to proceed past the
early return check. Apply the established mocking pattern that is already
documented in the terminal environment capabilities test suite.
---
Nitpick comments:
In `@packages/widgets/src/input/Button.ts`:
- Around line 260-264: The animation timer setup logic in the conditional block
checking isAnimating and _animationTimer is duplicating the functionality
already present in the _scheduleAnimation() method. Replace the inline
setTimeout code that sets _animationTimer, nullifies it after delay, and calls
markDirty() with a direct call to _scheduleAnimation() instead to consolidate
the animation scheduling logic into a single reusable method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e84684cd-c26a-4039-811f-9d8a0dca53db
📒 Files selected for processing (2)
packages/widgets/src/input/Button.test.tspackages/widgets/src/input/Button.ts
| it('focusing the button (isFocused = true) renders with hover background', () => { | ||
| const { button, screen } = renderButton('Click'); | ||
|
|
||
| button.isFocused = true; | ||
| button.render(screen); | ||
|
|
||
| expect(screen.back[1][1].bg).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Strengthen assertion to verify actual hover background color.
toBeDefined() passes for any non-undefined value, including { type: 'none' }. The test should verify the specific hover background color is applied when focused.
💡 Suggested improvement
it('focusing the button (isFocused = true) renders with hover background', () => {
const { button, screen } = renderButton('Click');
button.isFocused = true;
button.render(screen);
- expect(screen.back[1][1].bg).toBeDefined();
+ // Verify hover background is applied (default variant uses white hover bg)
+ expect(screen.back[1][1].bg).toEqual({ type: 'named', name: 'white' });
});As per coding guidelines: "Tests must be real. expect(true).toBe(true) and expect(widget).toBeDefined() are not tests. Assert observable behavior or rendered output."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('focusing the button (isFocused = true) renders with hover background', () => { | |
| const { button, screen } = renderButton('Click'); | |
| button.isFocused = true; | |
| button.render(screen); | |
| expect(screen.back[1][1].bg).toBeDefined(); | |
| }); | |
| it('focusing the button (isFocused = true) renders with hover background', () => { | |
| const { button, screen } = renderButton('Click'); | |
| button.isFocused = true; | |
| button.render(screen); | |
| // Verify hover background is applied (default variant uses white hover bg) | |
| expect(screen.back[1][1].bg).toEqual({ type: 'named', name: 'white' }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/input/Button.test.ts` around lines 199 - 206, The
assertion in the test case for focusing the button (where isFocused = true) is
too weak. Replace the `expect(screen.back[1][1].bg).toBeDefined()` assertion
with a stronger check that verifies the actual hover background color value is
applied correctly when the button is focused. Instead of just checking if bg is
defined, assert that bg contains the expected hover background color value (not
just any truthy value like { type: 'none' }).
Source: Coding guidelines
| it('startPress triggers re-render and does not throw', () => { | ||
| const { button } = renderButton('Click'); | ||
|
|
||
| expect(() => button.startPress()).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Test is a placeholder — does not verify re-render behavior.
The test name claims startPress triggers re-render but the assertion only checks that no exception is thrown. This violates the guideline against placeholder tests.
Either rename to reflect actual behavior tested, or add meaningful assertions that verify the press animation schedules a re-render (similar to the startHover test pattern).
💡 Suggested improvement
-it('startPress triggers re-render and does not throw', () => {
- const { button } = renderButton('Click');
-
- expect(() => button.startPress()).not.toThrow();
-});
+it('startPress schedules a re-render via markDirty after the animation tick', async () => {
+ // Mock caps.motion to enable animations
+ const { button } = renderButton('Click');
+ const markSpy = vi.spyOn(button, 'markDirty');
+
+ button.startPress();
+ await new Promise(resolve => setTimeout(resolve, 30));
+
+ expect(markSpy).toHaveBeenCalled();
+});As per coding guidelines: "Tests must be real... Do not leave placeholder tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/input/Button.test.ts` around lines 208 - 212, The test
`'startPress triggers re-render and does not throw'` has a misleading name
because it only verifies that `startPress()` does not throw an exception, but
does not actually verify that a re-render is triggered. Either rename the test
to accurately reflect what is being tested (e.g., `'startPress does not
throw'`), or add meaningful assertions that verify the re-render behavior by
checking that the press animation schedules a re-render, following the same
pattern used in the `startHover` test.
Source: Coding guidelines
1 participant
Closes #1735
Changes
startHover()/endHover()/startPress()methods toButtonfor hover/press animation statesanimationMs, default 200ms)_renderSelf()and drive the hover animation, sinceisFocusedis set directly by the focus managerprefersReducedMotion()— no animation timers run if the user has reduced motion enabledTesting
Summary by CodeRabbit
animationMstiming option.startHover()andendHover()to control hover animation.onPresswhen activating via Enter/Space.