Fix #2036: Remove underline and transition from ToggleableList #2051
Conversation
Summary by CodeRabbit
WalkthroughRemoves transition/underline hover styles from ToggleableList item buttons and updates the corresponding unit test to assert only hover background changes. No logic or API changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (3)
frontend/src/components/ToggleableList.tsx (2)
38-41: Add type="button" to be explicit and avoid accidental form submissionDefensive best practice; prevents this button from becoming a form submitter if the component is ever embedded in a form.
Apply this diff:
<button key={index} + type="button" className="rounded-lg border border-gray-400 px-3 py-1 text-sm hover:bg-gray-200 dark:border-gray-300 dark:hover:bg-gray-700" onClick={() => handleButtonClick({ item })}
40-40: Optional a11y: consider adding focus-visible ringTo aid keyboard users without introducing transitions, consider focus-visible utilities, e.g.:
Add to className (example):
- focus-visible:outline-none
- focus-visible:ring-2 focus-visible:ring-offset-1 focus-visible:ring-gray-400
- dark:focus-visible:ring-gray-500
This keeps hover behavior per spec while improving focus indication.
frontend/__tests__/unit/components/ToggleableList.test.tsx (1)
172-193: Optional: query by role and add a generic guard for transitionsMinor resilience improvements:
- Prefer getByRole('button', { name: 'React' }) for accessibility-aligned querying.
- Add a generic assertion to catch any future transition utilities, e.g.:
expect(button.className).not.toMatch(/\b(transition|duration|ease|transform|scale-)\b/)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ToggleableList.test.tsx(2 hunks)frontend/src/components/ToggleableList.tsx(1 hunks)
🔇 Additional comments (2)
frontend/src/components/ToggleableList.tsx (1)
40-40: Hover underline/transition removed as requested — LGTMMatches #2036: no underline, no scaling/transition; only background changes on hover (including dark mode). Behavior and API unaffected.
frontend/__tests__/unit/components/ToggleableList.test.tsx (1)
172-193: Tests correctly assert the new styling contractPositive checks for expected classes and explicit negatives for underline/transition/scale confirm the intended hover-only background behavior. Good alignment with component changes.
|
2 participants
Resolves #2036
Description:
This PRs changes the ToggleableList component so that buttons no longer show an underline or transition effects when hovered only the background color changes and adds corresponding tests to ensure this behavior.
Checklist
make check-testlocally; all checks and tests passed.