fix(ui): update the free trial badge spacing and rendering#8712
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1573490 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
📝 WalkthroughWalkthroughThis PR updates LineItems.Title: it increases the inline gap between icon and text/badge from t.space.$1 to t.space.$2, and when a badge is present it is rendered wrapped in a Box (badge ? {badge} : null). It also adds a changeset marking Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/elements/LineItems.tsx (1)
116-116: Consider adding tests for this spacing change.While the spacing adjustment looks appropriate, consider adding snapshot tests or visual regression tests to prevent unintended changes to the badge layout in the future. For styling changes like this, snapshot tests can capture the rendered structure and catch regressions.
As per coding guidelines, tests should be added to cover changes when possible.
🤖 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/ui/src/elements/LineItems.tsx` at line 116, Add tests that lock the spacing change in LineItems.tsx so future regressions are caught: create a unit snapshot test that renders the LineItems component (or the specific Badge/line-item fragment that uses gap: t.space.$2) using your test renderer (e.g., react-testing-library + jest) and assert the output with toMatchSnapshot, and/or add a visual regression/storybook snapshot that captures the rendered badge layout; reference the LineItems component and the gap: t.space.$2 style when naming the test to make the intent obvious.
🤖 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 `@packages/ui/src/elements/LineItems.tsx`:
- Line 116: Add tests that lock the spacing change in LineItems.tsx so future
regressions are caught: create a unit snapshot test that renders the LineItems
component (or the specific Badge/line-item fragment that uses gap: t.space.$2)
using your test renderer (e.g., react-testing-library + jest) and assert the
output with toMatchSnapshot, and/or add a visual regression/storybook snapshot
that captures the rendered badge layout; reference the LineItems component and
the gap: t.space.$2 style when naming the test to make the intent obvious.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b489f214-26ed-4b1a-a48c-0032398ea4b0
📒 Files selected for processing (1)
packages/ui/src/elements/LineItems.tsx
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
Snapi: no API changes detected in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/elements/LineItems.tsx (1)
127-136: ⚡ Quick winAdd a regression test for badge spacing/alignment behavior.
This change alters badge rendering structure and vertical alignment (Line 127-Line 136). Please add/adjust a UI test so spacing/alignment regressions are caught.
As per coding guidelines, "
**/*: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes."🤖 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/ui/src/elements/LineItems.tsx` around lines 127 - 136, The badge rendering change in LineItems.tsx (the conditional that renders the Span when the badge prop is present) can affect vertical spacing/alignment; add a UI regression test that mounts the LineItems component with and without the badge prop and asserts the badge is rendered as an inline-flex element with the expected vertical spacing (margin-block: -2px) and proper alignment relative to sibling text/avatars; target the Span wrapper (the Span element in LineItems) and check computed style or class that ensures display:inline-flex and marginBlock '-2px', and include a failing-case snapshot or DOM assertion so future changes to LineItems/Span will be caught.
🤖 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/ui/src/elements/LineItems.tsx`:
- Around line 128-135: The new themable wrapper Span around {badge} lacks an
elementDescriptor, so add an elementDescriptor prop to the Span (e.g.,
elementDescriptor={{ name: 'lineItemBadge' }} or using your naming convention)
so consumers can target/customize this node; update the Span usage in the
LineItems component to include that elementDescriptor and ensure the descriptor
name is documented/consistent with other elementDescriptor names in the project.
---
Nitpick comments:
In `@packages/ui/src/elements/LineItems.tsx`:
- Around line 127-136: The badge rendering change in LineItems.tsx (the
conditional that renders the Span when the badge prop is present) can affect
vertical spacing/alignment; add a UI regression test that mounts the LineItems
component with and without the badge prop and asserts the badge is rendered as
an inline-flex element with the expected vertical spacing (margin-block: -2px)
and proper alignment relative to sibling text/avatars; target the Span wrapper
(the Span element in LineItems) and check computed style or class that ensures
display:inline-flex and marginBlock '-2px', and include a failing-case snapshot
or DOM assertion so future changes to LineItems/Span will be caught.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f45c0b72-63b9-4f70-9d27-24e9f153dc0b
📒 Files selected for processing (1)
packages/ui/src/elements/LineItems.tsx
| <Span | ||
| sx={{ | ||
| display: 'inline-flex', | ||
| marginBlock: '-2px', | ||
| }} | ||
| > | ||
| {badge} | ||
| </Span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Expose the new badge wrapper through an element descriptor.
Line 128-Line 135 adds a new themable wrapper <Span>, but it has no elementDescriptor, so consumers can’t target/customize this node consistently.
Suggested change
{badge ? (
<Span
+ elementDescriptor={descriptors.lineItemsTitleBadge}
sx={{
display: 'inline-flex',
marginBlock: '-2px',
}}
>
{badge}
</Span>
) : null}As per coding guidelines, "Use element descriptors for all themable elements by applying elementDescriptor prop to components."
🤖 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/ui/src/elements/LineItems.tsx` around lines 128 - 135, The new
themable wrapper Span around {badge} lacks an elementDescriptor, so add an
elementDescriptor prop to the Span (e.g., elementDescriptor={{ name:
'lineItemBadge' }} or using your naming convention) so consumers can
target/customize this node; update the Span usage in the LineItems component to
include that elementDescriptor and ensure the descriptor name is
documented/consistent with other elementDescriptor names in the project.
| <Span localizationKey={title} /> | ||
| {badge} | ||
| {badge ? ( | ||
| <Span |
There was a problem hiding this comment.
instead of wrapping in a span with custom styles, I think wrapping in <Box> should fix this, since the Box will inherit the height of the flex container, and the Badge will then be allowed to shrink to its intrinsic height.
There was a problem hiding this comment.
So just something like this?
{badge ? (
<Box>
{badge}
</Box>
) : null}
|
Break Check: no API changes detected across the tracked packages. |
l-armstrong
force-pushed
the
lamone/bill-1800-fix-free-trial-badge-in-checkout-and-price-transition-modal
branch
from
38b7aaa to
efec22f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.changeset/empty-spiders-happen.md:
- Line 2: Update the changeset to use a patch-level semver bump instead of a
major one: locate the line containing "'`@clerk/ui`': major" in the .changeset
file and change the value from major to patch (or minor if you intentionally
added a non-breaking feature), then save the file so the changeset reflects the
correct non-breaking bug-fix scope.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 95e1ea22-fbbc-4f46-bf6c-03ebece02ea5
📒 Files selected for processing (1)
.changeset/empty-spiders-happen.md
l-armstrong
changed the title
l-armstrong
changed the title
l-armstrong
changed the title
l-armstrong
changed the title
l-armstrong
changed the title
l-armstrong
force-pushed
the
lamone/bill-1800-fix-free-trial-badge-in-checkout-and-price-transition-modal
branch
from
259b7f1 to
1573490
Compare
| ) : null} | ||
| <Span localizationKey={title} /> | ||
| {badge} | ||
| {badge ? <Box>{badge}</Box> : null} |
There was a problem hiding this comment.
@l-armstrong why'd we add a Box wrapper here? This shouldn't be needed from my testing. We are just adding more spacing between the title and the badge?
There was a problem hiding this comment.
@alexcarpenter Previously, I tried fixing an issue I noticed with the free trial badge during checkouts when testing in staging. The badge occupied more space than necessary. Initially, I tried fixing it with a span and negative margins, but it was suggested by @dstaley to use a Box wrapper instead.
jacekradko
deleted the
lamone/bill-1800-fix-free-trial-badge-in-checkout-and-price-transition-modal
branch
6 participants
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change