Skip to content

[$1000] [Refactor] NumberWithSymbolForm — composition over configuration #97970

Open
3 of 11 issues completed

Description

@melvin-bot

Problem

src/components/NumberWithSymbolForm.tsx is a component with 61 public props, 20 of them booleans, driving 3 distinct render paths. It carries two parallel APIs for the same trailing button (currency/onSymbolButtonPress vs currencyButtonLabel/onCurrencyButtonPress), the currency button implemented 4 times and the flip button 3 times. Several props are dead regardless of caller input: onChangeAmount is hardcoded internally, caller-passed selection and rightHandSideComponent are ignored, prefixStyle and shouldAllowFocusInLandscapeMode are fixed inside the component. In the displayAsTextInput path, roughly 32 of the 61 props have no effect, and the type signature doesn't say so.

Why this is a business problem

150 commits from 38 authors in the last 12 months, 60 of them fixes or reverts, with no dedicated test coverage — correctness is only checked indirectly through whatever screen a change happens to touch. 8 direct call sites, each passing 12 to 44 props. Amount-related changes carry review and regression cost disproportionate to their size, and that cost grows with every new flag.

Proposed fix

Move from configuration flags to explicit composition via primitives, so call sites build what they need.

<NumberForm value={amount} onInputChange={handleAmount}>  // root component
  <NumberForm.Input symbol="$" position="prefix" /> // text input with symbol display
  <NumberForm.FlipButton /> // toggle +/- sign button
  <NumberForm.CurrencyButton currency="USD" onPress={handleCurrency} /> // currency/unit selector button
  <NumberForm.BigNumberPad onNumberPressed={handleNumber} />  // big number pad for touch input
</NumberForm>

// focused preset - distance input: combines primitives for distance/unit suffix pattern.
<NumberFormDistance
  value={distance}
  unit={distanceUnit}
  onInputChange={setDistance}
  onSubmit={save}
/>

Each primitive owns a minimal prop set and its own tests. NumberFormDistance is a thin wrapper combining primitives with a limited prop set to prevent duplication across components that reuse the same pattern. During the refactor, we'll likely discover additional preset patterns worth extracting as similar wrappers. NumberWithSymbolForm stays, marked deprecated, so migration happens incrementally without breaking existing screens.

cc @mountiny @roryabraham

Issue OwnerCurrent Issue Owner: @bernhardoj

Activity

  1. self-assigned this
    on Aug 6, 2026
  2. changed the title [-][Refactor] NumberWithSymbolForm — composition over configuration[/-] [+][$1000] [Refactor] NumberWithSymbolForm — composition over configuration[/+] on Aug 6, 2026
  3. melvin-bot commented on Aug 6, 2026

    @melvin-bot
    Author

    ⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

  4. mountiny commented on Aug 6, 2026

    @mountiny
    Contributor
  5. roryabraham commented on Aug 6, 2026

    @roryabraham
    Contributor

    @mountiny do you want to be reviewer on this one or shall I?

  6. mountiny commented on Aug 7, 2026

    @mountiny
    Contributor

    I am happy to be the reviewer 🙌

  7. jmgraa commented on Aug 10, 2026

    @jmgraa
    Contributor

    NumberWithSymbolForm.tsx - migration plan

    Rough plan for how I want to split this up. Additional PRs/issues may be added when migration reveals required refactoring or missing coverage.

    Composition layer

    • 1. Define the NumberForm API and implement NumberForm.Input

      Define the root NumberForm contract and implement NumberForm + NumberForm.Input together so the types do not churn in a follow-up.

      • Root props: value, onInputChange, negativeMode, children.
      • NumberForm.Input — numeric input and symbol display, including parsing, formatting, validation, selection, keyboard, locale, and ref behavior moved out of NumberWithSymbolForm.
    • 2. Implement NumberForm.FlipButton and NumberForm.CurrencyButton

      Add both controls with minimal prop sets, focused tests, and shared behavior extracted from the duplicated button implementations in NumberWithSymbolForm.

    • 3. Implement NumberForm.BigNumberPad and layout composition

      Add NumberForm.BigNumberPad and compose all primitives through NumberForm across portrait, landscape, touch, and non-touch layouts. Remove the remaining duplicated layout/button paths from the old component once the new composition covers them. Ship pad and layout tests with this PR.

    Migration and documentation

    Preset wrappers (for example NumberFormDistance) are not built ahead of time. Add a thin preset in the same PR as the first call site that needs that repeated pattern, and only when composing the primitives inline would otherwise copy the same structure. Further presets may be added the same way whenever migration reveals another justified pattern.

    • 4. Mark NumberWithSymbolForm deprecated and attach the migration guide

      Once the composition API is usable, mark NumberWithSymbolForm deprecated and document the V1 prop-to-composition mapping, direct composition examples, any known or expected presets, and the verification expected for each migrated call site.

    • 5. Migrate call sites by pattern

      Open focused issues/PRs against these files. Prefer migrating hubs before leaf pages that only type against their refs. Each issue should state the selected composition (or preset), preserved behavior, and tests.

      • AmountForm hub:

        • src/components/AmountForm.tsx
        • follow-up type update: src/pages/ChronosScheduleOOOPage.tsx (uses AmountForm + NumberWithSymbolFormRef)
      • MoneyRequestAmountInput hub:

        • src/components/MoneyRequestAmountInput.tsx
        • follow-up type/ref update: src/pages/iou/MoneyRequestAmountForm.tsx (wraps MoneyRequestAmountInput + NumberWithSymbolFormRef)
      • Distance / unit-suffix — add NumberFormDistance here with the first migrated file, then reuse it for the rest:

        • src/pages/iou/request/step/DistanceManualTabContent.tsx
        • src/pages/iou/request/step/IOURequestStepDistanceManual.tsx
        • follow-up type/ref update: src/pages/iou/request/step/IOURequestStepDistance.tsx
      • Simple text / unit inputs → NumberForm with Input and only the required children:

        • src/pages/iou/request/step/IOURequestStepHours.tsx
        • src/pages/workspace/taxes/WorkspaceCreateTaxValuePage.tsx
        • src/components/MoneyRequestConfirmationList/sections/TaxFields.tsx
        • src/components/MoneyRequestConfirmationList/sections/AmountField.tsx
        • related test updates: tests/ui/IOURequestStepHoursTest.tsx
      • Form InputComponent registration:

        • src/pages/workspace/taxes/ValuePage.tsx
        • src/components/Form/types.ts

      Stories under src/stories/NumberWithSymbolForm.stories.tsx are removed in cleanup, not migrated.

    Cleanup

    • 6. Remove NumberWithSymbolForm

      After all direct and indirect consumers migrate, remove the deprecated component, old prop types, duplicated layout/button logic, compatibility code, stories, tests, and remaining registrations.

    I'll be doing 1–4 myself since they define the shared composition and migration path. Once the guide (or first migration mapping) is ready, 5 can be picked up by others as separate issues.

    I'll drop an update here every Friday with what's done, what's left, newly discovered scope, and anything blocking me.

  8. 19 remaining items

  9. jmgraa commented on Sep 11, 2026

    @jmgraa
    Contributor

    Implementation update

    After several rounds of discussion, implementation review, and investigation of the existing behavior, we have aligned on the target architecture. The work is now being implemented incrementally in a sequence of focused PRs, with each step validating part of the new composition before the next migration begins.

    The original plan was to introduce a single NumberForm component with a NumberForm.Input primitive. During implementation, it became clear that NumberWithSymbolForm currently combines two substantially different interaction models:

    • a standard text-based numeric field;
    • a currency-entry experience with sign handling, currency controls, selection management, and a number pad.

    These flows have different requirements and should not continue to share one large, tightly coupled component. The new architecture therefore introduces NumericField for standard inputs and NumericInput for the currency-entry flow. Their common parsing, formatting, validation, locale, external-value, and selection behavior is provided by NumericEditingController.

    The work originally planned under #98169 was split as the architecture became clearer. NumericField and NumericEditingController are now implemented and have been integrated into the standard AmountForm input path. NumericInput is being developed separately so that the more complex signed currency-entry behavior can be handled without adding further complexity to the standard field.

    #98170 is currently in progress. The currency and flip controls are being implemented as NumericInput primitives. Alongside the controls themselves, this work covers sign ownership and synchronization, including partial edits, full-value replacement, pasted positive and negative values, clearing the amount, rejected minus-sign edits, and typing the minus sign at different caret positions.

    The original acceptance criteria described gradually migrating the internals of NumberWithSymbolForm. Based on the implementation work, this cannot be done safely while keeping the existing component structure. Its input, sign, currency, selection, layout, and number-pad paths are too closely connected, and extracting them one by one would leave the same complexity in place while making behavior harder to verify.

    Instead, the current PRs are migrating small, isolated usage paths directly away from NumberWithSymbolForm. This lets us validate NumericField and NumericInput independently and gradually expand their coverage. The non-touch/web tax-value flow has already been used as an early validation of the NumericInput approach, while the native touch flow remains on the legacy component until the number-pad and layout work is complete.

    The next step is #98171. Once the new primitives cover the required layouts and number-pad behavior, NumberWithSymbolForm will be rewritten as a compatibility adapter over the new composition, preserving its public props and ref contract. Only then will the remaining hubs and call sites be migrated in the later sub-issues, followed by the final cleanup and removal of the legacy component.

    I will update each sub-issue description when it is merged so that it reflects the actual implementation and final scope.

  10. mountiny commented on Sep 11, 2026

    @mountiny
    Contributor

    Thanks! Ping me in slack if there will be anything waiting on me to review

  11. jmgraa commented on Sep 17, 2026

    @jmgraa
    Contributor

    @mountiny @Expensify/design

    Question about the future of these screens and the refactor

    A while ago, the full-screen "amount + number pad" screen shown when creating a manual expense was replaced with a simpler inline text field as part of the confirmation screen. It no longer uses a number pad or takes over the full screen: #100215.

    The full-screen number-pad design is still used for distance rate, hours, tax rate, and general amount entry across most expense flows. Out of these, only two screens need a minus sign rendered as a separate element next to the currency symbol. That combination is where the extra complexity and bug history lie:

    Scenario 1 - editing an amount in a Self-DM 1. Open your Self-DM chat (the chat with your own name).
    2. Create an expense for any amount if you do not have one yet.
    3. Tap the expense, then tap the *Amount* row.
    ios_1.mov
    web_1.mov
    Scenario 2 - editing an amount on a workspace expense 1. Open a workspace expense (Collect/Control).
    2. Tap the expense, then tap the *Amount* row.
    ios_2.mov
    web_2.mov

    Since the full-screen amount screen used when creating an expense was replaced with inline input, we wanted to check before extending the negative-amount handling: are more of these full-screen amount screens planned for migration to the inline (or another) design soon? This is entirely your call. From an engineering perspective, if these screens are likely to be migrated in the near term, it may not be worth adding separate-minus-sign handling or migrating them as part of this refactor, if the current design will soon be retired.

    Questions

    A) Is the full-screen number-pad design going away soon for more screens beyond expense creation?

    • If yes:
      • Include that work in this refactor directly: replace all full-screen amount screens with the inline (or another suggested) design instead of building a new full-screen component.
    • If no:
      • Only these two negative-amount screens need a decision; everything else will keep the full-screen design long term. See part B.

    B) Assuming only these two negative-amount screens are in question, what should we do with them specifically?

    1. Keep them on the current component unchanged, but mark it as deprecated until a decision is made. Build the new version for every other full-screen amount screen.
    2. Migrate them to the same simple inline text field used on the expense-creation screen—no full screen and no separate minus sign.
    3. Add separate-minus-sign handling to the new component so these two screens receive the same treatment as the others.

    We strongly recommend A (if the full-screen design is going away across more screens) or B2 (if those two negative-amount screens are the only ones in question). Choosing B2 would allow us to remove NumberWithSymbolForm completely, standardize the design of input fields across the app, and avoid maintaining extra minus-sign logic for just two screens. Our fallback is B1: it also avoids the extra logic, but NumberWithSymbolForm would have to remain in the repository until these screens are migrated. B3 is a valid option, but it requires additional logic; if you plan to add more full-screen number-pad screens, however, this option becomes preferred and necessary. Ultimately, this choice is entirely up to you and your team depending on your product vision.

    Related bug history for context: #94255, #78330, #84668, #53441, #65147/#75304/#86394.

  12. shawnborton commented on Sep 17, 2026

    @shawnborton
    Contributor

    are more of these full-screen amount screens planned for migration to the inline (or another) design soon?

    I could see this at some point, but we don't have any concrete plans yet, so I wouldn't block our progress on this assumption as we don't know when we'll actually do it. cc @JmillsExpensify for thoughts too.

  13. shawnborton commented on Sep 17, 2026

    @shawnborton
    Contributor

    I think ideally we would just keep things looking and feeling as they are today.

  14. jmgraa commented on Sep 17, 2026

    @jmgraa
    Contributor

    Thanks @shawnborton, understood!

    @mountiny could you help us decide between B1 and B3 from the implementation perspective? My preference is B1, as described above, but I’m happy to proceed with either option.

  15. mountiny commented on Sep 17, 2026

    @mountiny
    Contributor

    I can see why b1 is a fallback, but since we dont have concrete plans to remove this, maybe it would be worth to spend the time to do b3?

  16. jmgraa commented on Oct 2, 2026

    @jmgraa
    Contributor

    Implementation update

    Quick update on the progress and next steps:

    1. Status & Layout PRs

    Following @mountiny's suggestion, I went with B3 (supporting separate minus-sign handling and full-screen layout). I am currently wrapping up the final stages of the layout implementation and aim to submit all the foundation/layout PRs next week. Once those are merged, we will be ready to begin the screen-by-screen migration.

    2. NumberWithSymbolForm as an Adapter: Risk & QA Scope

    We initially considered turning NumberWithSymbolForm into an internal adapter backed by the new primitives before migrating callers. However, upon deeper inspection, doing so in production carries substantial risk:

    • Architectural & State Differences: The refactor separates concerns cleanly (e.g., parent-owned sign handling via hooks, separating layout from inputs, distinct inline vs. full-screen lifecycles). Forcing all legacy callers through an internal adapter requires reconciling conflicting legacy behaviors inside a single compatibility wrapper.
    • Massive QA Bottleneck: Merging an adapter into main would switch all ~15 disparate call sites across the app (distance, hours, tax rates, splits, standard amounts, OOO schedules, card limits, etc.) to the new internals at once. A single PR would require exhaustive regression testing across all platforms and screens simultaneously.

    3. Proposed Strategy: Verification PoC Branch & Storybook

    To guarantee zero visual or behavioral regressions without putting production at risk, I propose the following plan:

    1. Merge Foundation / Layout PRs: Land the clean primitives (NumericInput, NumericField, layout, buttons, etc.) on main.
    2. Open a non-mergeable PoC branch / PR for review:
      • Create an adapter implementation on a separate branch solely for demonstration and review (this PR will not be merged).
      • Visual Parity: Showcase a comprehensive side-by-side Storybook comparison covering Web, Mobile Portrait, and Mobile Landscape, comparing the legacy component vs. the new composition to prove pixel-perfect visual fidelity.
      • Test Parity: Run the 90 characterization tests introduced in Foundation 0 (NumberWithSymbolFormTest) against the adapter to demonstrate that baseline behavior and edge cases pass.
    3. Migrate Domain Tests: In the final cleanup phase, migrate the domain test cases from NumberWithSymbolFormTest to the corresponding new primitives (NumericInput, NumericField, etc.) to ensure all tested behavior is permanently preserved and enhanced at the unit level.
    4. Smooth Screen-by-Screen Migration: With the PoC validated, we can migrate screens incrementally in isolated batches/PRs. Each screen will be cleanly migrated to the new primitives and QA’d in isolation without touching unrelated flows.

    @mountiny Looking forward to your thoughts and whether you're aligned with this approach!

  17. melvin-bot commented on Oct 2, 2026

    @melvin-bot
    Author

    This issue has not been updated in over 15 days. @mountiny, @bernhardoj, @jmgraa eroding to Monthly issue.

    P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

  18. mountiny commented on Oct 5, 2026

    @mountiny
    Contributor

    yeah that approach sounds good!

  19. jmgraa commented on Oct 9, 2026

    @jmgraa
    Contributor

    Hi everyone,
    First of all, apologies for the delay! I wanted to ensure the interface and architecture were kept as clean and simple as possible.

    The good news is that all the PRs covering the full scope are now prepared:

    • Logic refactoring / cleanups
    • Fixed and polished BigNumberPad component along with the layout updates
    • Storybook stories
    • Migration to the adapter pattern

    Next steps & current status:

    1. Right now, there are 2 PRs waiting for review that need to be merged first:
    2. Once those are merged, @bernhardoj can return to reviewing Add NumericInput BigNumberPad and consolidate full-screen layout #102140
    3. After that, I will be rolling out the subsequent PRs in sequence.

    Thanks for your patience!

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

Metadata

Metadata

Labels

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions