fix(rn): baseline-align and prevent clipping of Amount currency/decimal#3488
Open
rohitr-raz wants to merge 3 commits into
Open
fix(rn): baseline-align and prevent clipping of Amount currency/decimal#3488rohitr-raz wants to merge 3 commits into
rohitr-raz wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: ec5ba55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Contributor
|
✅ PR title follows Conventional Commits specification. |
Contributor
PR DescriptionFix React Native baseline alignment and clipping of Amount component's currency symbol and decimal. Description of Changes
Note for ReviewerThe Review Effort4 Flow DiagramNot applicable Generated by Slash Reviewer |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ec5ba55:
|
Contributor
There was a problem hiding this comment.
AI Code Review
Related Suggestions
These files are not part of this PR but may need attention:
packages/blade/src/components/Amount/Amount.tsx:419 [CODE_QUALITY, importance: 7]
Left/right currency BaseText blocks are ~15-line duplicates; extract to a local helper
packages/blade/src/components/Amount/Amount.tsx:450 [CODE_QUALITY, importance: 7]
Replace duplicate left/right currency JSX with
renderCurrencySymbolhelper calls
Tools Used
Skills: pre-mortem
Contributor
PR Risk Assessment: 🟡 MEDIUMRule Source: repo_skill ( ReasoningThis PR fixes a React Native rendering bug in Sub-Agent Attribution
Assessed by Slash Reviewer |
Collaborator
🛡️ Coverage ReportSummaryFull Coverage Details |
Contributor
|
I think the clipping part is only for storybook. Can you check in a react native app and use different sizes for amount ? |
Switch the native Amount layout from flex-end (which aligns view bottoms and misplaces smaller glyphs) to baseline alignment, matching the web path. Add includeFontPadding:false so Android reports accurate baselines, and give the currency symbol and decimal their own proportional lineHeight to avoid clipping large display-size glyphs without shifting the baseline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
c3ccf2d to
ec5ba55
Compare
Contributor
|
🤖 Slash AI Review has been triggered. View execution logs |
Contributor
There was a problem hiding this comment.
✨ Agentic PR Review ✨
UI Review
🔗 Storybook: Preview
✅ 9 passed
Passing checks (9)
| Check |
|---|
| ✅ Amount Default |
| ✅ Amount Sizes |
| ✅ Amount Currency |
| ✅ Amount All Intents |
| ✅ Amount Strike Through |
| ✅ Amount With Text |
| ✅ Amount i18n Locales |
| ✅ Amount Affix Subtle Off |
| ✅ Amount Kitchen Sink |
CI / Sanity
✅ 12 passed · ❌ 2 failed · ⏭️ 2 skipped
| Check | Problem |
|---|---|
| Run Tests (1) | Snapshot mismatch in src/components/BottomSheet/tests/BottomSheet.native.test.tsx (2 failing tests: 'should render with header' and 'should render empty header'). Snapshots show unexpected width="100%" prop added to a View inside BottomSheetHeader that was not updated.Suggestion: Run yarn run test:react-native -u to regenerate all RN snapshots and commit the updated files. |
| Run Tests (4) | Snapshot mismatch in src/components/Dropdown/tests/Dropdown.native.test.tsx and src/components/Input/DropdownInputTriggers/tests/AutoComplete.native.test.tsx. Both show unexpected width="100%" prop added to inner Views that were not updated.Suggestion: Run yarn run test:react-native -u to regenerate all RN snapshots and commit the updated files. |
Passing checks (12)
⚠️ React Native snapshots not updated: the PR introduces layout changes that affect RN rendering in BottomSheet, Dropdown, and AutoComplete components (likely via a shared layout primitive). The stored snapshots were not regenerated. Runyarn run test:react-native -uand commit the updated snapshot files.
| @@ -20,7 +20,6 @@ import { assignWithoutSideEffects } from '~utils/assignWithoutSideEffects'; | |||
| import { throwBladeError } from '~utils/logger'; | |||
Contributor
There was a problem hiding this comment.
🔵 [MINOR] · usecase-critique
Problem: includeFontPadding is passed as style={{ includeFontPadding: false } as never} in multiple places, casting to never to bypass BaseText's style type. If BaseText is a shared primitive, other Android text rendering issues will hit the same wall and require the same cast.
Suggestion: Add includeFontPadding as an explicit optional prop on BaseText (typed as a platform-conditional) so it can be passed cleanly without a type cast.
| @@ -20,7 +20,6 @@ import { assignWithoutSideEffects } from '~utils/assignWithoutSideEffects'; | |||
| import { throwBladeError } from '~utils/logger'; | |||
Contributor
There was a problem hiding this comment.
🔵 [MINOR] · code-quality-critique
Problem: getPlatformType() === 'react-native' is evaluated independently in both AmountValue and _Amount components, creating two separate isReactNative variables that could diverge in future refactors.
Suggestion: Lift the constant to module scope: const IS_REACT_NATIVE = getPlatformType() === 'react-native'; and reference it in both components.
| opacity={isAffixSubtle ? opacity[800] : 1} | ||
| style={currencyFontProps.style} | ||
| style={ | ||
| (isReactNative ? { includeFontPadding: false } : currencyFontProps.style) as never |
Contributor
There was a problem hiding this comment.
🟠 [MAJOR] · code-quality-critique
Problem: When isReactNative is true, currencyFontProps.style is unconditionally replaced with { includeFontPadding: false }, discarding any hardcoded fontSize override. For isAffixSubtle=false with type='heading' or type='display', getCurrencyFontProps() returns a style containing a hardcoded pixel fontSize (from currencyHardcodedSizes) that ensures the currency symbol matches the Figma spec. Dropping that on RN means the currency symbol renders at the token font size instead of the designer-specified size, causing visual misalignment on those variants.
Suggestion: Merge both objects instead of replacing: style={(isReactNative ? { ...currencyFontProps.style, includeFontPadding: false } : currencyFontProps.style) as never}. This preserves the hardcoded fontSize on Android/iOS while adding the includeFontPadding fix. Apply the same fix to the right-side currency symbol block at line ~486.
| opacity={isAffixSubtle ? opacity[800] : 1} | ||
| style={currencyFontProps.style} | ||
| style={ | ||
| (isReactNative ? { includeFontPadding: false } : currencyFontProps.style) as never |
Contributor
There was a problem hiding this comment.
🟠 [MAJOR] · code-quality-critique
Problem: Same issue as line 460 — the suffix-right (currencyPosition === 'right') BaseText for the currency symbol also replaces currencyFontProps.style with { includeFontPadding: false } when isReactNative is true, losing the hardcoded pixel fontSize override required for non-subtle heading/display types.
Suggestion: Use style={(isReactNative ? { ...currencyFontProps.style, includeFontPadding: false } : currencyFontProps.style) as never} here as well, mirroring the fix for the left-side currency symbol.
| <BaseText | ||
| fontWeight={weight} | ||
| fontSize={affixFontSize} | ||
| lineHeight={affixFontSize} |
Contributor
There was a problem hiding this comment.
🔵 [MINOR] · code-quality-critique
Problem: lineHeight={affixFontSize} passes a fontSize token key (e.g. 25, 75, 100) as a lineHeight token key. Both namespaces share the same numeric keys but resolve to different pixel values — lineHeights[25]=13px vs fontSize[25]=10px. The PR comment describes this as the affix's 'own proportional lineHeight', but the resolved lineHeight is actually the lineHeights token at that key, not the fontSize value itself.
Suggestion: Consider using the matching lineHeight token explicitly or add a comment clarifying that affixFontSize keys are intentionally used as lineHeight token keys because the two maps share key names, and lineHeight[N] > fontSize[N] provides just enough clearance.
| marginRight="spacing.1" | ||
| fontWeight={weight} | ||
| fontSize={currencyFontProps.fontSize} | ||
| lineHeight={isReactNative ? currencyFontProps.fontSize : undefined} |
Contributor
There was a problem hiding this comment.
🔵 [MINOR] · code-quality-critique
Problem: lineHeight={isReactNative ? currencyFontProps.fontSize : undefined} — currencyFontProps.fontSize can be undefined (the typing allows it). Passing lineHeight={undefined} on RN causes BaseText to fall back to the default lineHeight token (100 → 20px), potentially introducing vertical whitespace for small currency font sizes.
Suggestion: Add a fallback: lineHeight={isReactNative ? (currencyFontProps.fontSize ?? affixFontSize) : undefined} or assert non-null with a comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the React Native (Android) rendering of the
Amountcomponent, where the ₹ currency symbol and the decimal (.79) were clipped at large display sizes and visually misaligned with the integer.alignItems: 'flex-end'(aligns view bottoms, which misplaces smaller glyphs) toalignItems: 'baseline', matching the existing web path.includeFontPadding: falseto the text elements so Android reports an accurate baseline (its default font padding skews baseline alignment).lineHeight(their own font-size token rather than the integer's larger one) — enough room to avoid clipping large display-size glyphs without shifting the baseline.Web rendering is unchanged; all new values are gated behind
isReactNative.##Description:
before:
after:
Test plan
type/sizecombinations (body, heading, display) that ₹, the integer, and.79sit on one baseline.displaymedium/large/xlarge.🤖 Generated with Claude Code