Skip to content

Migrate Button ro ButtonComposed (batch 10) - #98861

Merged
mountiny merged 5 commits into
Expensify:mainfrom
software-mansion-labs:Guccio163/button/afterBatchesCleanup
Aug 20, 2026
Merged

Migrate Button ro ButtonComposed (batch 10)#98861
mountiny merged 5 commits into
Expensify:mainfrom
software-mansion-labs:Guccio163/button/afterBatchesCleanup

Conversation

@Guccio163

@Guccio163 Guccio163 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

This is an extra, 10th batch in the ongoing effort to migrate all direct usages (import Button from '@components/Button') to the new composed ButtonComposed, allowing the old Button component to eventually be deprecated. This PR migrates 44 leftover files that appeared after the original 282-file census, plus BaseWidgetItem's buttonProps public API and its 14 callers. Adds accessibilityLabel support to ButtonIcon to preserve NumberWithSymbolForm's icon labels.

Moreover this PR changes BaseWidgetItem API proposed in this comment, from buttonProps to buttonVariant as only buttonVariant has been passed in this prop.

Fixed Issues

$ #98878
PROPOSAL:

Tests

For every migrated button, verify that behaviour is unchanged from before the migration:

  1. Label renders correctly and is not clipped or truncated.
  2. Variant/color is correct — success = green, danger = red, default = grey.
  3. Size is correct — SMALL vs MEDIUM vs LARGE height/padding matches the previous look.
  4. Icon renders on the correct side (left icon before the label, right icon after it) and is centered/grouped with the label.
  5. Disabled state — the button greys out and is non-interactive when its precondition isn't met (offline, throttled, no write access).
  6. Loading state — the spinner replaces the label and the button is non-interactive while a request is in flight (isLoading).
  7. Press action — tapping fires the same documented action as before (navigates / submits / opens modal / opens popover).
  8. Enter key — where the old button had pressOnEnter, pressing Enter still submits exactly once.
  9. Forwarded ref — where a ref is forwarded, its consumer still works: popovers open anchored to the button, not pinned to a window corner.

Below are test steps for testing a representative migrated Button use-case: BaseWidgetItem:

  1. Open expensify app (on any platform)
  2. Enter any unused email (f.ex. buttontests+randomnumber@gmail.com) → Continue
  3. Click Join (since this is a new account)
  4. After redirection to 'Home' you should see 'Validate your account' text in the Time sensitive' section, with green button 'Validate'
  5. Click the 'Validate' button, check if the 'Validate your account' page opens with validation/magic code.
  6. Check if the 'Validate' button is styles correctly.

Offline tests

QA Steps

Same as tests.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-08-18.at.14.16.18.mov

Follow-up cleanup after the 9 batch PRs: migrates 44 leftover files
that appeared after the original 282-file census, plus BaseWidgetItem's
buttonProps public API and its 14 callers. Adds accessibilityLabel
support to ButtonIcon to preserve NumberWithSymbolForm's icon labels.
@melvin-bot

melvin-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@Guccio163
Guccio163 marked this pull request as ready for review August 18, 2026 12:39
@Guccio163
Guccio163 requested review from a team as code owners August 18, 2026 12:39
@melvin-bot
melvin-bot Bot requested review from ZhenjaHorbach and trjExpensify and removed request for a team August 18, 2026 12:39
@melvin-bot

melvin-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

@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]

@melvin-bot
melvin-bot Bot removed the request for review from a team August 18, 2026 12:39
@Guccio163

Copy link
Copy Markdown
Contributor Author

@ZhenjaHorbach should be ready for a review, I'll change that buttonProps -> buttonVariant part later, after Vit's opinion

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/components/AccountSwitcher.tsx 82.35% <ø> (ø)
src/components/ApprovalWorkflowSection.tsx 80.00% <ø> (ø)
src/components/Banner.tsx 100.00% <ø> (ø)
src/components/BaseWidgetItem.tsx 100.00% <100.00%> (ø)
src/components/BiometricsTestToolRow.tsx 100.00% <100.00%> (ø)
src/components/BookCallButton.tsx 100.00% <100.00%> (ø)
src/components/BookTravelButton.tsx 83.95% <100.00%> (+2.37%) ⬆️
...rc/components/Button/ButtonDisabledWhenOffline.tsx 0.00% <ø> (ø)
...omponents/ButtonComposed/primitives/ButtonIcon.tsx 92.30% <100.00%> (ø)
src/components/ConfirmationPage.tsx 92.10% <ø> (ø)
... and 50 more
... and 156 files with indirect coverage changes

@ZhenjaHorbach

Copy link
Copy Markdown
Contributor

@Guccio163
Let's link this PR to this issue

@trjExpensify

Copy link
Copy Markdown
Contributor

PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself.

@trjExpensify
trjExpensify removed their request for review August 18, 2026 14:27
…-fork into Guccio163/button/afterBatchesCleanup
Every real caller only ever spread buttonProps to set a single variant,
so replace the generic Partial<ButtonProps> prop-spread with a typed
buttonVariant prop instead.
@Guccio163

Copy link
Copy Markdown
Contributor Author

@ZhenjaHorbach I just applied changes proposed in this comment

@ZhenjaHorbach

ZhenjaHorbach commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
2026-08-20.11.44.54.mov
Android: mWeb Chrome
2026-08-20.11.37.23.mov
iOS: HybridApp
2026-08-20.11.40.56.mov
iOS: mWeb Safari
2026-08-20.11.39.19.mov
MacOS: Chrome / Safari
2026-08-20.11.33.36.mov

@ZhenjaHorbach

ZhenjaHorbach commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Overall, the changes look good
But found a small mismatch on BookCallButton
We have different paddings on dev and staging

2026-08-20.11.30.44.mov

@Guccio163

Copy link
Copy Markdown
Contributor Author

@ZhenjaHorbach thanks, I'll do it soon; BTW since we are targeting button wrappers to use variant instead of success and danger I wanted to migrate ConfirmModal's usages and wrappers, but ended up with ~100 files; Would you rather add it in this PR, or separate into another follow-up, potentially taking care of other Button wrappers still using success/button. I think that the second option would be cleaner LMKWYT 👀

@ZhenjaHorbach

ZhenjaHorbach commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Yeah
This PR is too big
Let's create separate subissues!

@Guccio163

Copy link
Copy Markdown
Contributor Author

We have different paddings on dev and staging

@ZhenjaHorbach I can't measure this Button's horizontal paddings on staging/prod because I don't have Concierge pass, but I checked that dev correctly applies styles

Screenshot 2026-08-20 at 13 04 18

In default size (medium) it should have 12px on the left and 16px on the right - Left is from paddingHorizontal 12px, Right is from paddingHorizontal 12px+4px ButtonText paddingHorizontal, gap is fine too. I verified that BookCallButton doesn't receive custom styles anywhere, so it should be like it's now 👍

@ZhenjaHorbach

Copy link
Copy Markdown
Contributor

We have different paddings on dev and staging

@ZhenjaHorbach I can't measure this Button's horizontal paddings on staging/prod because I don't have Concierge pass, but I checked that dev correctly applies styles

Screenshot 2026-08-20 at 13 04 18 In default size (medium) it should have 12px on the left and 16px on the right - Left is from paddingHorizontal 12px, Right is from paddingHorizontal 12px+4px ButtonText paddingHorizontal, gap is fine too. I verified that BookCallButton doesn't receive custom styles anywhere, so it should be like it's now 👍

I think it's a bit of a different case

Now on dev, we have 12 horizontal padding
Снимок экрана — 2026-08-20 в 13 12 30

And n staging, we have 16 horizontal padding
Снимок экрана — 2026-08-20 в 13 12 16

But now checked different places, and 12 px is okay
So probably we can leave this

@melvin-bot
melvin-bot Bot requested a review from mountiny August 20, 2026 11:18

@mountiny mountiny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZhenjaHorbach @Guccio163 I think we should make sure it matches production

@ZhenjaHorbach

ZhenjaHorbach commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@ZhenjaHorbach @Guccio163 I think we should make sure it matches production

I suppose now it matches the similar buttons that have the same paddings
So, as for me, everything is fine

@Expensify/design
But can you confirm which horizontal padding we should have for BookCallButton?

Like on staging
image

On, like on dev
image

And we have similar buttons where we also use 12

image Снимок экрана — 2026-08-20 в 15 57 08 Снимок экрана — 2026-08-20 в 15 57 28

@Guccio163

Guccio163 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

When using icon + text it should be asymmetric 12px on icon's side and 16px on text's, just as on the last screenshot (right 4px coming from Text component). On your screenshot with avatar instead of icon, text doesn't add said padding, because there is normal Text instead of Button.Text used (custom children). In this scenario I would fix text-side padding to be 16px and either treat Avatar like a normal icon and leave the 12px or (since it looks like it's bigger than medium icon) also bump avatar-side padding to the 16px, just like bigger icons (in bigger buttons) receive. WDYT @ZhenjaHorbach @mountiny ?

@ZhenjaHorbach

ZhenjaHorbach commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

When using icon + text it should be asymmetric 12px on icon's side and 16px on text's, just as on the last screenshot (right 4px coming from Text component). On your screenshot with avatar instead of icon, text doesn't add said padding, because there is normal Text instead of Button.Text used (custom children). In this scenario I would fix text-side padding to be 16px and either treat Avatar like a normal icon and leave the 12px or (since it looks like it's bigger than medium icon) also bump avatar-side padding to the 16px, just like bigger icons (in bigger buttons) receive. WDYT @ZhenjaHorbach @mountiny ?

In my opinion, the avatar should use the same styles as the icon
But I already pinged the design team
So let's wait 😁

@shawnborton

Copy link
Copy Markdown
Contributor

Agree, I would think we'd just reuse the same styles for simplicity's sake.

@Guccio163

Guccio163 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

But I already pinged the design team

Good call, I think I would go with that logic too. Overall good catch though, since text-side padding is 100% wrong

@Guccio163

Copy link
Copy Markdown
Contributor Author

Agree, I would think we'd just reuse the same styles for simplicity's sake.

Thanks @shawnborton, I'll fix it right away and we'll see if it looks right 👌

@Guccio163

Copy link
Copy Markdown
Contributor Author

I fixed (and simplified while on it) BookCallButton's use-case, now with style identical as with icon:
Screenshot 2026-08-20 at 16 07 27
Screenshot 2026-08-20 at 15 30 17

@shawnborton

Copy link
Copy Markdown
Contributor

Looks good 👍

@melvin-bot
melvin-bot Bot requested a review from mountiny August 20, 2026 14:47

@mountiny mountiny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everyone

@mountiny
mountiny merged commit 0714c20 into Expensify:main Aug 20, 2026
36 of 38 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants