Migrate Button to ButtonComposed (batch 8) - #97089
Conversation
mikolajpochec
changed the title
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
This button keeps raw <Text> children instead of Button.Text, and that silently shrinks it by 8px.
The legacy Button took the children path with text='' and icon=null, so getButtonPaddingStyle saw symmetric icons, returned undefined, and the button kept buttonSmall.paddingHorizontal: 12. ButtonComposed always applies horizontalPaddingBySize[SMALL] = ph2 = 8 (src/components/ButtonComposed/Button.tsx:96). The compensating ph1 lives inside ButtonText (src/components/ButtonComposed/primitives/ButtonText.tsx:51), and this call site never renders ButtonText.
Net effect: 12 -> 8 per side, so the button is 8px narrower than on main. For every other call site in this PR the geometry works out identically, precisely because ButtonText.ph1 makes up the difference.
| <Button | ||
| small | ||
| size={CONST.BUTTON_SIZE.SMALL} | ||
| style={style} | ||
| onPress={() => setIsHidden(!isHidden)} | ||
| testID="moderationButton" | ||
| sentryLabel={CONST.SENTRY_LABEL.ATTACHMENT_CAROUSEL.MODERATION_BUTTON} | ||
| > | ||
| <Text | ||
| style={[styles.buttonSmallText, styles.userSelectNone]} | ||
| dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}} | ||
| > | ||
| {isHidden ? translate('moderation.revealMessage') : translate('moderation.hideMessage')} | ||
| </Text> | ||
| </Button> |
There was a problem hiding this comment.
Same issue as in FloatingMessageCounter: raw <Text> children skip ButtonText, so this button loses the compensating ph1 and ends up 8px narrower (12 -> 8 horizontal padding per side).
There was a problem hiding this comment.
Same issue as in FloatingMessageCounter: raw <Text> children skip ButtonText, so this button loses the compensating ph1 and ends up 8px narrower (12 -> 8 horizontal padding per side).
| <Button | ||
| small | ||
| size={CONST.BUTTON_SIZE.SMALL} | ||
| style={styles.pl2} | ||
| onPress={() => removeEvent(reportID, action.reportActionID, event.id, events)} | ||
| > | ||
| <Text style={styles.buttonSmallText}>{translate('common.remove')}</Text> | ||
| </Button> |
There was a problem hiding this comment.
Same issue as in FloatingMessageCounter: raw <Text> children skip ButtonText, so this button loses the compensating ph1 and ends up 8px narrower (12 -> 8 horizontal padding per side).
| @@ -41,14 +41,23 @@ type FloatingPillButtonProps = { | |||
| textStyle?: StyleProp<TextStyle>; | |||
| }; | |||
There was a problem hiding this comment.
With variant in place, success, danger, iconFill and textStyle on FloatingPillButtonProps are redundant.
There was a problem hiding this comment.
One caveat: Button.Icon at size=SMALL renders iconSizeExtraSmall (12px, because isButtonIcon is set), while the current raw <Icon size={SMALL}> renders 16px. The icon will shrink, so it needs a visual check.
There was a problem hiding this comment.
I got rid of getButtonVariant, but I'm leaving rest of params as they are, since changing Text and Icon to ButtonComposed equivalents breaks UI a bit
| onPress={togglePINVisibility} | ||
| medium | ||
| /> | ||
| size={CONST.BUTTON_SIZE.MEDIUM} |
There was a problem hiding this comment.
MEDIUM is already the default (src/components/ButtonComposed/Button.tsx:28)
| medium | ||
| text={translate('workspace.common.viewTransactions')} | ||
| icon={expensifyIcons.MoneySearch} | ||
| size={CONST.BUTTON_SIZE.MEDIUM} |
There was a problem hiding this comment.
MEDIUM is already the default (src/components/ButtonComposed/Button.tsx:28)
| medium | ||
| text={translate('workspace.common.viewTransactions')} | ||
| icon={expensifyIcons.MoneySearch} | ||
| size={CONST.BUTTON_SIZE.MEDIUM} |
There was a problem hiding this comment.
MEDIUM is already the default (src/components/ButtonComposed/Button.tsx:28)
There was a problem hiding this comment.
I think this is correct, but just to be safe, I'll point out that this file still uses the old Button.
There was a problem hiding this comment.
It seems that src/pages/settings/Wallet/ExpensifyCardPage/index.tsx is a subject of batch 7. I migrated only CardDetailsActionButton in this file due to changes made in its definition in src/pages/settings/Wallet/CardDetailsActionButtons.tsx.
There was a problem hiding this comment.
We should probably change this to variant?: typeof CONST.BUTTON_VARIANT.SUCCESS
…-fork into @mikolajpochec/button-migration-pr8
…-fork into @mikolajpochec/button-migration-pr8
dariusz-biela
left a comment
dariusz-biela
left a comment
There was a problem hiding this comment.
Nice work, the batch is in good shape.
| <Button | ||
| size={CONST.BUTTON_SIZE.SMALL} | ||
| style={style} | ||
| innerStyles={styles.ph3} |
There was a problem hiding this comment.
Could you add a short comment, for example:
// Raw children skip ButtonText and the ph1 it contributes, so the legacy 12px horizontal padding is restored here.
innerStyles={styles.ph3}| <Button | ||
| size={CONST.BUTTON_SIZE.SMALL} | ||
| style={styles.pl2} | ||
| innerStyles={styles.ph3} |
There was a problem hiding this comment.
Same as in CarouselItem, please add the one line comment explaining that ph3 replaces the ph1 that ButtonText would normally contribute.
| @@ -90,6 +90,7 @@ function ChatMessageContent({action, policyID, reportID, originalReportID, displ | |||
| <Button | |||
| size={CONST.BUTTON_SIZE.SMALL} | |||
| style={[styles.mt2, styles.alignSelfStart]} | |||
| innerStyles={styles.ph3} | |||
There was a problem hiding this comment.
Same as in CarouselItem, please add the one line comment explaining that ph3 replaces the ph1 that ButtonText would normally contribute.
There was a problem hiding this comment.
This is the same compensation as in CarouselItem, ChatMessageContent and ChronosOOOListActions, but through a different mechanism. Here it is ph1 on your own View (8 + 4), there it is innerStyles={styles.ph3} (8 overridden by 12). Both land on the legacy 12px, so this is purely about consistency. Could we use one mechanism in all four places? contentContainerStyle={styles.ph1} is the most honest one, because it literally replaces the ph1 that ButtonText would have added, and the prop already exists on ButtonProps. Whichever you pick, the comment from the CarouselItem thread applies here as well.
| /** Inner button variant **/ | ||
| variant?: ButtonVariant; |
There was a problem hiding this comment.
NAB: Closes with **/ instead of */
| // Visual variant of the button (only success is supported) | ||
| variant?: typeof CONST.BUTTON_VARIANT.SUCCESS; |
There was a problem hiding this comment.
// Submit buttons support the default and success styles; danger is not a valid submit state.
variant?: typeof CONST.BUTTON_VARIANT.SUCCESS;| @@ -50,6 +50,7 @@ function CarouselItem({item, onPress, isFocused, isModalHovered, reportID}: Caro | |||
| <Button | |||
There was a problem hiding this comment.
On main the raw Text was a child of a column container, so a long label wrapped. ButtonComposed wraps children in a flexRow and row items default to flexShrink: 0, so it now overflows instead. Button.Text sets flexShrink1 for this reason. Low risk with these short labels, but worth a check in de and a styles.flexShrink1 if it overflows. Same in ChatMessageContent.tsx:96 and ChronosOOOListActions.tsx:69.
There was a problem hiding this comment.
Thanks! I tested all three buttons in German and with an artificially long label and saw no overflow, so I decided I'll leave it as it is
…-fork into @mikolajpochec/button-migration-pr8
|
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…-fork into @mikolajpochec/button-migration-pr8
Guccio163
left a comment
Guccio163
left a comment
There was a problem hiding this comment.
Generally looks good to me, I left some comments and remember to apply Darek's ones
| size={CONST.BUTTON_SIZE.SMALL} | ||
| onPress={onPress} | ||
| sentryLabel={CONST.SENTRY_LABEL.REPORT.FLOATING_MESSAGE_COUNTER} | ||
| innerStyles={styles.ph1} |
There was a problem hiding this comment.
Could use a comment like in CarouselItem.tsx, ChronosOOOListActions.tsx, ChatMessageContent.tsx
There was a problem hiding this comment.
BTW I feel like it should be styles.pr3: Firstly we need to compensate with extra padding only on text's side (that's why right instead of horizontal) and Secondly original padding that we aim to achieve is 12px, so pr3, instead of pr1.
| /> | ||
| stayNormalOnDisable | ||
| > | ||
| {/* Transparent loading content still affects layout. Mount the icon only after loading so it does not widen the button. */} |
There was a problem hiding this comment.
Unnecessary comment in my opinion. Text's state was previously directly dependent only on showLoading, now it's also on isAnimationRunning. I'd rename isShowingSubmittedState to shouldShowIcon or even move it inline since icon's only place that uses both isAnimationRunning and showLoading directly
| <Button.Icon | ||
| src={icons.BackArrow} | ||
| fill={theme.text} | ||
| /> |
There was a problem hiding this comment.
Nice find, we should look into it 👀
| <Button | ||
| small | ||
| size={CONST.BUTTON_SIZE.SMALL} | ||
| innerStyles={[styles.arrowIcon]} |
There was a problem hiding this comment.
NIT:
| innerStyles={[styles.arrowIcon]} | |
| innerStyles={styles.arrowIcon} |
| <Button.Icon | ||
| src={icons.ArrowRight} | ||
| fill={theme.white} | ||
| /> | ||
| </Button> |
There was a problem hiding this comment.
Same as in previous comment with icon hover fill
3 participants
Explanation of Change
This is batch 8 (PR8 of 9) in the ongoing effort to migrate all direct
<Button>usages (import Button from '@components/Button') to the new composedButtonComposed, allowing the old Button component to eventually be deprecated. This batch covers the "text + icon · plain + icon-only + no-content" shape and includes ~32 files / ~40 button instances.Some files listed in the migration issue are not included here because they were renamed, removed, or already migrated:
src/components/AvatarCropModal/AvatarCropModal.tsx— renamed toAvatarCropView.tsxand already migrated.src/pages/settings/Agents/AgentsListRow.tsx— no longer exists.src/pages/settings/Agents/AgentsPage.tsx— already migrated.This batch also includes small migrations of Button wrapper components, where direct migration was simpler than adding adapters. This applies to
AnimatedSubmitButton,AnimatedSettlementButton,CardSectionButton, andCardDetailsActionButton.Fixed Issues
$ #95177
PROPOSAL: #83762 (comment)
Tests
For every migrated button, verify that behaviour is unchanged from before the migration:
success= green,danger= red, default = grey.isLoading).Moreover, I divided the migrated buttons into several categories, based on their shape and rendering behaviour, and recorded one representative for each. For each category, detailed test steps are described in the sections below:
Button.Textto hold the content row open — the lone icon must still center)<Text>child instead of<Button.Text>(the child keeps its own styling — highest-risk visual check).Textchild into the composedButtonvariantmapped from booleans via a helper (success / danger / neutral branches)For each of these categories, detailed test steps are described in sections below:
Plain left-icon + text button
Preconditions
Test steps
Expected behavior
The Plus icon sits immediately left of the "New" label — icon + text grouped and centered — as a green
success button, identical to a production build. Pressing it opens the new-workspace flow.
Icon-only button (attachment carousel arrows)
Preconditions
None — any chat works (your self-DM is fine).
Test steps
carousel has more than one page.
non-touch); on native / touch they show immediately then auto-hide after ~3s (tap to bring them back).
between the two images.
Expected behavior
Each arrow is a small round button with a single centered icon tinted
theme.text— no clipping or off-centerglyph — visually identical to production; the tooltips read "Previous" / "Next" and tapping navigates.
Raw
<Text>child button (reveal/hide flagged message)This button passes a bare
<Text>as its child rather than<Button.Text>, so the check is that the label'sfont size, weight, color, horizontal padding and centering are pixel-identical to production.
Test steps — real path (needs a second account)
severity that results in hidden message. (You cannot flag your own message, which is why the second account is required.)
"Hide message".
Expected behavior
The button label renders identical to production in both toggle states — same small-button text style, padding
and centering as before the migration; no shift in color/weight/size.
No-content wrapper forwarding a
.Textchild (Subscription payment buttons)The "Retry payment" / "Authenticate payment" buttons render through the
CardSectionButtonwrapper,which forwards its
.Textchild into the composedButton. This is web-only —CardSectionButtonhas areturn nullnative no-op (pre-existing), so verify on desktop/mobile web.Preconditions
return nullno-op on native).Path A — surface the button by altering code (recommended)
settings/subscription).CardSection).src/pages/settings/Subscription/CardSection/CardSection.tsx, stubconst billingStatus = {isRetryAvailable: true};just above the return — this forces the "Retry payment"guard (
billingStatus?.isRetryAvailable !== undefined, ~line 250).size=LARGE.Path B — reach the button from a real backend state (no code changes)
settings/subscription).3DS
authentication_requiredstate the "Authenticate payment" button renders. Neither state can betriggered from the app UI — this path depends on the actual backend condition.
Expected behavior
The
CardSectionButton.Textlabel renders centered inside the full-widthsize=LARGEbutton, identical toproduction; the disabled (offline) and loading (retry pending) states behave as before.
Floating message counter
Test steps
genuinely unread messages sit below, otherwise neutral grey "Latest messages".
Expected behavior
Each pill's background color matches its state exactly like production.
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
button_custom_children_android.mp4
button_only_icon_android.mp4
Android: mWeb Chrome
button_custom_children_android_web.mp4
button_only_icon_android_web.mp4
iOS: Native
button_custom_children_ios.mp4
button_only_icon_ios.mp4
iOS: mWeb Safari
button_custom_children_ios_web.mp4
button_only_icon_ios_web.mp4
MacOS: Chrome / Safari
button_only_icon_web.mp4