fix(ramp): migrate sell amount keypad to MMDS BottomSheetDialog - #33311
fix(ramp): migrate sell amount keypad to MMDS BottomSheetDialog#33311georgewrmarshall wants to merge 10 commits into
Conversation
metamask-ci
Bot
added
the
team-design-system
PR template — items to address before "Ready for review"Warnings — informational, address before merging:
See docs/readme/ready-for-review.md for the full Definition of Ready for Review. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Keypad lost horizontal padding
- Added px-4 to the bottom sheet content container and removed the inner padding wrapper so the keypad aligns with prior layout and no longer touches screen edges.
Or push these changes by commenting:
@cursor push 948e0b18ee
Preview (948e0b18ee)
diff --git a/app/components/UI/Ramp/Aggregator/Views/BuildQuote/BuildQuote.tsx b/app/components/UI/Ramp/Aggregator/Views/BuildQuote/BuildQuote.tsx
--- a/app/components/UI/Ramp/Aggregator/Views/BuildQuote/BuildQuote.tsx
+++ b/app/components/UI/Ramp/Aggregator/Views/BuildQuote/BuildQuote.tsx
@@ -1164,7 +1164,7 @@
true
}
>
- <Box twClassName="content-end gap-4 pt-4">
+ <Box twClassName="content-end gap-4 pt-4 px-4">
<QuickAmounts
isBuy={isBuy}
onAmountPress={handleQuickAmountPress}
@@ -1182,7 +1182,7 @@
isBuy ? currentFiatCurrency?.decimals : selectedAsset?.decimals
}
/>
- <Box twClassName="px-4">
+ <Box>
<Button
size={ButtonSize.Lg}
onPress={handleKeypadDone}You can send follow-ups to the cloud agent here.
| // Mock MMDS BottomSheetDialog so children render synchronously in tests. | ||
| jest.mock('@metamask/design-system-react-native', () => { | ||
| const actual = jest.requireActual('@metamask/design-system-react-native'); | ||
| const MockReact = jest.requireActual('react'); | ||
|
|
||
| return { | ||
| ...actual, | ||
| BottomSheetDialog: MockReact.forwardRef( | ||
| ( | ||
| { | ||
| children, | ||
| onClose, | ||
| }: { children: React.ReactNode; onClose?: () => void }, | ||
| dialogRef: React.Ref<{ onCloseDialog: () => void }>, | ||
| ) => { | ||
| MockReact.useImperativeHandle(dialogRef, () => ({ | ||
| onCloseDialog: () => onClose?.(), | ||
| })); | ||
| return children; | ||
| }, | ||
| ), | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
@cursoragent remove the mock and run unit tests this is likely not needed. Can we use a test id instead to assert that the BottomDialog is there?
There was a problem hiding this comment.
Addressed the review feedback:
-
Removed the
BottomSheetDialogmock fromBuildQuote.test.tsx— the reviewer was right; it wasn't needed. -
Added a test ID —
build-quote-amount-keypad-bottom-sheeton theBottomSheetDialog, exported asBuildQuoteSelectors.AMOUNT_KEYPAD_BOTTOM_SHEET. -
Updated the focus test —
"shows and hides the live input cursor based on focus"now asserts the bottom sheet is present when the amount input is focused and absent after tapping Done. -
Fixed keypad padding (Bugbot) — moved
px-4to the outerBoxwrapper so the numpad matches the Swaps pattern.
All 48 unit tests pass without the mock. Changes pushed to PR #33311.
🧪 Flaky unit test detectionRun history flaky detectionHistorical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow. Failures / runs sampled per window:
AI-detected flaky patterns
|
|
Replace the custom Reanimated keypad overlay in Ramp Aggregator BuildQuote with MMDS BottomSheetDialog, following the Swaps numpad migration pattern. - Remove keypadContainer animation and surface styling hacks - Use isKeypadOpen state for bottom sheet lifecycle - Drop custom QuickAmounts background; MMDS owns the surface - Mock BottomSheetDialog in unit tests for synchronous rendering - Fix native-asset gas tests to render after mock setup Fixes TMCU-1084 Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
Address PR review: drop MMDS BottomSheetDialog jest mock and assert keypad sheet presence with build-quote-amount-keypad-bottom-sheet. Also restore horizontal padding on keypad content (px-4). Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
Cover hardware back dismissal, selector navigation closing the sheet, and sell-flow bottom sheet open/close to meet SonarCloud new-code coverage threshold. Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
Co-authored-by: George Marshall <georgewrmarshall@users.noreply.github.com>
a92de0a to
82a8dea
Compare
Align sell/buy keypad presets with MMDS Button Lg sizing and flex layout used by Swaps and the shared Ramp QuickAmounts. Co-authored-by: Cursor <cursoragent@cursor.com>
Swap quick amounts for Done in the same keypad slot, migrate selectors/buttons/text to MMDS, and update AccountSelector to SelectButton. Co-authored-by: Cursor <cursoragent@cursor.com>
Document the migration path on the legacy SelectorButton and note that Ramp AccountSelector already uses SelectButton. Co-authored-by: Cursor <cursoragent@cursor.com>
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| </Animated.View> | ||
| > | ||
| <Box twClassName="content-end px-4 gap-4 pt-4"> | ||
| {amount && amount !== '0' ? ( |
There was a problem hiding this comment.
Matches the Swaps keypad pattern: quick-amount presets and Done share one slot so the sheet stays one row shorter. Done only appears once the amount is non-zero, same condition BridgeView uses for confirm vs quick picks.
| testID={BuildQuoteSelectors.AMOUNT_KEYPAD_BOTTOM_SHEET} | ||
| isInteractable={false} | ||
| onClose={handleKeypadClose} | ||
| onStartShouldSetResponder={() => |
There was a problem hiding this comment.
Required so taps on non-pressable keypad chrome do not bubble to BottomSheetDialog and dismiss the sheet. Same responder guard used by SwapsKeypad after the MMDS BottomSheetDialog migration.
| isBuy | ||
| ? currentFiatCurrency?.symbol | ||
| : `${selectedAsset?.symbol}-crypto` | ||
| {isKeypadOpen ? ( |
There was a problem hiding this comment.
Sheet is mounted only while open so MMDS owns lifecycle, surface, and safe-area instead of the old always-mounted Animated.View translateY overlay. isKeypadOpen is kept separate from amountFocused so the amount highlight can clear without fighting sheet close timing.
| <SelectorButton | ||
| accessibilityRole="button" | ||
| accessible | ||
| <SelectButton |
There was a problem hiding this comment.
Region and fiat chips use MMDS SelectButton instead of Base SelectorButton so select triggers share the same API and surface as other recent migrations. value/placeholder keep emoji and currency labels accessible without custom children wrappers.
| size={ButtonSize.Sm} | ||
| label={label} | ||
| variant={ButtonVariant.Secondary} | ||
| size={ButtonSize.Lg} |
There was a problem hiding this comment.
Lg sizing matches Swaps quick picks and the shared Ramp QuickAmounts. The old Sm size was 32px and looked undersized next to the Done CTA inside the MMDS sheet.
|
|
||
| return ( | ||
| <SelectorButton | ||
| <SelectButton |
There was a problem hiding this comment.
Keeps Ramp-specific account picker behavior (nav params, avatar seed, loading placeholder) while the trigger itself is MMDS SelectButton plus AvatarAccount. Only used from Aggregator BuildQuote.
| }); | ||
|
|
||
| /** | ||
| * @deprecated Please update your code to use `SelectButton` from `@metamask/design-system-react-native`. |
There was a problem hiding this comment.
Marks the legacy Base SelectorButton deprecated now that Aggregator BuildQuote and AccountSelector no longer depend on it. Points new select triggers at MMDS SelectButton, consistent with the component-library SelectButton deprecation.
| ).not.toBeOnTheScreen(); | ||
| }); | ||
|
|
||
| it('replaces quick amounts with Done confirm when an amount is entered', () => { |
There was a problem hiding this comment.
Locks in the exclusive quick-amount vs Done slot for buy flow. After a digit is entered, presets must disappear and the confirm testID must appear so keypad height and CTA behavior stay aligned with Swaps.
| navigateWithDetails( | ||
| navigation, | ||
| createTokenSelectModalNavigationDetails({ | ||
| handleKeypadClose(); |
There was a problem hiding this comment.
Closing the keypad before opening region/asset/fiat/payment selectors avoids stacking BottomSheetDialog over another modal. Covered by the new lifecycle tests for each selector entrypoint.
Restore the typed navigation helper for token/fiat/payment selectors so yarn lint:tsc accepts createNavigationDetails tuples. Co-authored-by: Cursor <cursoragent@cursor.com>
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
The No other smoke tags are impacted: the changes are isolated to the Ramp/Buy BuildQuote screen and its sub-components, with no changes to confirmations, network, accounts, swaps, or other flows. Performance Test Selection: |
2 participants
Description
Ramp Aggregator Sell (and Buy) amount entry used a custom
Animated.Viewkeypad overlay with ReanimatedtranslateYanimation and manual surface styling. This migrates the keypad to MMDSBottomSheetDialogfrom@metamask/design-system-react-native, following the Swaps numpad migration pattern in PR #33077.Changes:
BottomSheetDialogisKeypadOpenstate to manage bottom sheet lifecycle separately from amount highlight statekeypadContainer/ Reanimated animation hacks fromBuildQuote.styles.tsQuickAmountsso MMDS owns the elevated surfacebuild-quote-amount-keypad-bottom-sheettest IDChangelog
CHANGELOG entry: null
Related issues
Fixes: TMCU-1084
Manual testing steps
Screenshots/Recordings
N/A — UI migration to MMDS bottom sheet; QA screenshot requested in TMCU-1084 (Sell screen with amount keypad open).
Before
sell.before.mov
Comparing with swap
before.compare.mov
After
after720.mov
Comparing with swap
after.compare.mov
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an examplePre-merge reviewer checklist