Scale CTALinkOrButton chevron with size prop#2423
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request introduces dynamic chevron sizing to the CTALinkOrButton component. A new internal constant Possibly related PRs
🚥 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 |
Greptile SummaryFixes the hardcoded
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to chevron icon sizing and does not affect any existing visual output. All three size keys are covered in the new lookup table, the default size (medium) maps to the same class as before, tests verify each size variant, and the Storybook story covers the new large case. No existing callers combine size=large with a chevron, so there is no risk of unintended visual regression. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CTALinkOrButton\nsize prop] --> B{CTA_CHEVRON_SIZE_STYLES lookup}
B -->|small| C[size-2 — 8px]
B -->|medium| C
B -->|large| D[size-3 — 12px]
C --> E[FaChevronLeft / FaChevronRight\ncn className]
D --> E
Reviews (1): Last reviewed commit: "Scale CTALinkOrButton chevron with size ..." | Re-trigger Greptile |
The chevron rendered by withChevron / withBackChevron was hardcoded to size-2 (8px), which looks fine at the default medium size but appears tiny next to a size="large" hero CTA. Map the chevron size to the CTA size so large gets size-3 (12px). Small and medium are unchanged, so no existing usage shifts visually.
c27f558 to
68e9ffb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libraries/ui/src/CTALinkOrButton.test.tsx (1)
127-134: ⚡ Quick winOptional: assert element existence before accessing
classNamefor a clearer failure message.If the chevron icon element is ever not rendered,
?.classNamesilently producesundefined, and the assertion error readsexpected undefined to include 'size-2'— harder to diagnose than a dedicated existence check.♻️ Proposed refactor
test('chevron scales with size', () => { const { rerender } = render(<CTALinkOrButton size="small" withChevron>Click me</CTALinkOrButton>); - expect(document.querySelector('.cta-button__chevron-icon')?.className).includes('size-2'); + const getIcon = () => document.querySelector('.cta-button__chevron-icon'); + expect(getIcon()).toBeTruthy(); + expect(getIcon()!.className).includes('size-2'); rerender(<CTALinkOrButton size="medium" withChevron>Click me</CTALinkOrButton>); - expect(document.querySelector('.cta-button__chevron-icon')?.className).includes('size-2'); + expect(getIcon()!.className).includes('size-2'); rerender(<CTALinkOrButton size="large" withChevron>Click me</CTALinkOrButton>); - expect(document.querySelector('.cta-button__chevron-icon')?.className).includes('size-3'); + expect(getIcon()!.className).includes('size-3'); });🤖 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 `@libraries/ui/src/CTALinkOrButton.test.tsx` around lines 127 - 134, The test 'chevron scales with size' accesses document.querySelector('.cta-button__chevron-icon')?.className without confirming the element exists; update the assertions to first select the chevron element (using querySelector/getByRole/getByTestId) and assert its presence (e.g., expect(el).toBeTruthy() or toBeInTheDocument()) before checking its className includes the expected size, so each rerender uses a non-null element when verifying sizes for CTALinkOrButton.
🤖 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.
Nitpick comments:
In `@libraries/ui/src/CTALinkOrButton.test.tsx`:
- Around line 127-134: The test 'chevron scales with size' accesses
document.querySelector('.cta-button__chevron-icon')?.className without
confirming the element exists; update the assertions to first select the chevron
element (using querySelector/getByRole/getByTestId) and assert its presence
(e.g., expect(el).toBeTruthy() or toBeInTheDocument()) before checking its
className includes the expected size, so each rerender uses a non-null element
when verifying sizes for CTALinkOrButton.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27e8aaa7-c7e5-400a-b02f-e16474112c31
📒 Files selected for processing (3)
libraries/ui/src/CTALinkOrButton.stories.tsxlibraries/ui/src/CTALinkOrButton.test.tsxlibraries/ui/src/CTALinkOrButton.tsx
Description
The chevron rendered by
withChevron/withBackChevronwas hardcoded tosize-2(8px), which looks fine on the defaultmediumCTA but too small next to asize="large"hero CTA.(Noticed while reviewing #2417)
Issue
N/A
Developer checklist