refactor: Replace redesign confirmation BottomModal with BottomSheet #13268

digiwand · 2025-01-30T15:09:45Z

Description

Replaces BottomModal with BottomSheet in redesign Confirm page
- Add scroll support
- Remove min height 70% to prevent redesign Confirm page from showing excess space
- Allow BottomSheet styles (rounded corners, footer bottom styles, and device padding detection padding) rather than custom BottomModal styles
Add BottomSheet isBackgroundAlternative prop
- Added BottomSheet footer isBackgroundAlternative prop
- Note: I think we don't need the additional background colors for header and footer since the parent, BottomSheet animated view sets the background color. Once I removed them, it triggered many snapshot tests and code owner reviews from 5 additional teams. I reverted this in favor of the explicit isBackgroundAlternative prop
Update BlockaidBanner to remove surrounding margin to allow for better reusability
- Replace BlockaidBanner instances margin overrides with surrounding container margins
Fix TransactionReview BlockaidBanner negative padding → turn to positive padding to allow space between next element
Fix Confirm.test console.errors

Related issues

Fixes: #13267
Todo follow-up - Related Issue: #12656

Manual testing steps

Test BottomSheets and BlockaidBanner alerts work as expected. Test redesign confirmation page works as expected

Screenshots/Recordings

Before using BottomModal and no scroll behavior

CleanShot.2025-01-30.at.01.11.16.mp4

After using BottomSheet with scroll behavior

CleanShot.2025-01-30.at.02.18.53.mp4

Before 70% min height

After remove 70% min height

Before Transaction Review BlockaidBanner negative padding

After Transaction Review BlockaidBanner positive padding

Before Confirm.test console errors

After Confirm.test no console.errors

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

no need because BottomSheetDialog provides background color

- affects only old confirmations - fix marginBottom on TransactionReview

github-actions · 2025-01-30T15:09:58Z

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.

…ader" This reverts commit 04a974b.

…ative

sonarqubecloud · 2025-01-30T19:10:51Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2025-01-30T19:54:44Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 50594e4
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e9a940ea-e242-4a48-9a81-9bb840e70aa0

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

digiwand · 2025-01-30T19:56:39Z

app/components/Views/confirmations/Approval/__snapshots__/index.test.tsx.snap

@@ -595,7 +595,7 @@ exports[`Approval render matches snapshot 1`] = `
                                                  <View
                                                    style={
                                                      {
-                                                        "marginBottom": -8,
+                                                        "marginBottom": 8,


fixes missing padding between blockaid banner alert and account header

jpuri · 2025-01-31T10:41:59Z

app/components/Views/confirmations/components/SignatureRequest/index.js

+                securityAlertResponse={securityAlertResponse}
+                onContactUsClicked={this.onContactUsClicked}
+              />
+            </View>


I would suggest avoiding changing old pages

needed to make the change so that the reusable component does not apply it's own padding/margin. I think it's better to do this now than to continue this pattern of overriding the margin/padding through props

jpuri · 2025-01-31T10:43:11Z

A small thing I see missing in screenshots this small rectangle at top of bottom modal, its required by the designs:

jpuri · 2025-01-31T10:43:50Z

app/components/Views/confirmations/Confirm/Confirm.styles.ts

      paddingHorizontal: 16,
-      paddingVertical: 24,
-      minHeight: '70%',


I am not if removing minHeight is good idea, let's check once with @SayaGT

Before 70% min height

After remove 70% min height

I thought maybe it was to give more space for the expandable Message, but the expanded Message also seems condensed here

jpuri · 2025-01-31T10:45:10Z

app/components/Views/confirmations/components/TransactionReview/index.js

      marginHorizontal: 16,
-      marginBottom: -8,
+      marginBottom: 8,
    },
  });



Same, I would suggest avoiding any change to old pages.

this minor change fixes this

Before Transaction Review BlockaidBanner negative padding

**After Transaction Review BlockaidBanner positive padding **

jpuri · 2025-01-31T10:47:56Z

app/components/Views/confirmations/Confirm/Confirm.tsx

-    <BottomModal canCloseOnBackdropClick={false}>
-      <View style={styles.container} testID={approvalRequest?.type}>
-        <View>
-          <Title />


We now have a mix of use of BottomSheet and BottomModal in confirmation pages. Should we replace all occurence of BottomModal like these.

That's correct.

That was the original ticket found in the description

Todo follow-up - Related Issue: https://github.com/MetaMask/metamask-mobile/issues/12656

I created a separate issue ticket after working on this initial transition. Note: It is not possible to put a BottomSheet over a BottomModal too which is why this one needs to be done first

jpuri

Changes look good, I added some feedback that need to be addressed.

digiwand · 2025-01-31T13:48:15Z

app/components/Views/confirmations/Confirm/Confirm.tsx

-        <View>
-          <Title />
+    <BottomSheet
+      isInteractable={false}


re: grey rounded, rectangular UI at top of sheet
#13268 (comment)

If isInteractable is here true, then we show the gray rectangle. Dragging the UI down would close the UI. However, it doesn't seem we want the user to close the confirmation on drag down.

Interestingly, the BottomModal supports this behavior where it is draggable, but not closable:

CleanShot.2025-01-31.at.16.35.02.mp4

This is not a behavior with our DS BotttomSheet though. What is the expected behavior for the Confirm page here? Should we use BottomSheet isInteractable false as provided by DS? cc: @SayaGT @MetaMask/design-system @jpuri

… BottomSheet

brianacnguyen · 2025-02-13T16:01:40Z

app/component-library/components/BottomSheets/BottomSheet/BottomSheet.tsx

@@ -36,6 +36,7 @@ const BottomSheet = forwardRef<BottomSheetRef, BottomSheetProps>(
      children,
      onClose,
      onOpen,
+      isBackgroundAlternative = false,


@digiwand can you update this to bottomSheetDialogProps instead? lmk if you need to sync on this

hey @brianacnguyen, thank you for the review and reminder to push my updates! Apologies I didn't get to add this proposal before you reviewed. What do you think if we used stylesDialogSheet instead? This is because the style isn't for the general bottom sheet styles, but it's a specific style in BottomSheetDialog → sheet

updated 8cd072e

brianacnguyen

Request changes RE BottomSheet

digiwand added 3 commits January 30, 2025 02:23

refactor: rm background color form bottom sheet footer and header

04a974b

no need because BottomSheetDialog provides background color

refactor: use BottomSheet for redesign Confirm

a7c0698

refactor: replace BlockaidBanner style prop uses

f1defbf

- affects only old confirmations - fix marginBottom on TransactionReview

metamaskbot added the team-confirmations Push issues to confirmations team label Jan 30, 2025

digiwand added 2 commits January 30, 2025 19:22

test: update snapshots following BottomSheet updates

44837f7

test:fix: redesign Confirm mock SafeAreaProvider

3605a39

digiwand marked this pull request as ready for review January 30, 2025 18:22

digiwand requested review from a team as code owners January 30, 2025 18:22

digiwand added 3 commits January 30, 2025 21:40

Revert "refactor: rm background color form bottom sheet footer and he…

f9d3150

…ader" This reverts commit 04a974b.

refactor: BottomSheet replace styleAnimatedView -> isBackgroundAltern…

60757f0

…ative

test:fix: restore BottomSheet header and footer background

50594e4

digiwand mentioned this pull request Jan 30, 2025

feat: QR hardware signing in new designs #13261

Merged

7 tasks

digiwand added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jan 30, 2025

digiwand commented Jan 30, 2025

View reviewed changes

jpuri reviewed Jan 31, 2025

View reviewed changes

jpuri requested changes Jan 31, 2025

View reviewed changes

digiwand commented Jan 31, 2025

View reviewed changes

digiwand added 10 commits February 8, 2025 01:30

Merge branch 'main' into refactor-signature-use-bottom-sheet

3c4a247

fix: merge conflicts

02bbf25

test: rm async and update phrases Confirm.test

58ec799

test: rm unused getAddressAccountType mock Confirm.test

a7ea0fe

test: rm unnecessary beforeEach clearAllMocks

52761af

test: Confirm.test add more mocks to remove console.errors

40d16ad

test: Confirm.test rm missing act warning

07e7f1f

Merge branch 'main' into refactor-signature-use-bottom-sheet

9479133

refactor: revert isBackgroundAlternative -> stylesDialogSheet Confirm…

8cd072e

… BottomSheet

test:fix: bypass lint empty function

2605bca

brianacnguyen reviewed Feb 13, 2025

View reviewed changes

brianacnguyen requested changes Feb 13, 2025

View reviewed changes

fix: Confirm merge conflicts padding and height

42a4559

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Replace redesign confirmation BottomModal with BottomSheet #13268

refactor: Replace redesign confirmation BottomModal with BottomSheet #13268

digiwand commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 30, 2025

sonarqubecloud bot commented Jan 30, 2025

github-actions bot commented Jan 30, 2025 •

edited by metamaskbot

Loading

digiwand Jan 30, 2025

jpuri Jan 31, 2025

digiwand Jan 31, 2025

jpuri commented Jan 31, 2025 •

edited

Loading

jpuri Jan 31, 2025

digiwand Jan 31, 2025

digiwand Jan 31, 2025

jpuri Jan 31, 2025

digiwand Jan 31, 2025

jpuri Jan 31, 2025

digiwand Jan 31, 2025

jpuri left a comment

digiwand Jan 31, 2025 •

edited

Loading

brianacnguyen Feb 13, 2025

digiwand Feb 13, 2025 •

edited

Loading

brianacnguyen left a comment

refactor: Replace redesign confirmation BottomModal with BottomSheet #13268

Are you sure you want to change the base?

refactor: Replace redesign confirmation BottomModal with BottomSheet #13268

Conversation

digiwand commented Jan 30, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before using BottomModal and no scroll behavior

After using BottomSheet with scroll behavior

Before 70% min height

After remove 70% min height

Before Transaction Review BlockaidBanner negative padding

**After Transaction Review BlockaidBanner positive padding **

Before Confirm.test console errors

After Confirm.test no console.errors

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 30, 2025

sonarqubecloud bot commented Jan 30, 2025

Quality Gate passed

github-actions bot commented Jan 30, 2025 • edited by metamaskbot Loading

Bitrise

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpuri commented Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before 70% min height

After remove 70% min height

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before Transaction Review BlockaidBanner negative padding

**After Transaction Review BlockaidBanner positive padding **

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpuri left a comment

Choose a reason for hiding this comment

digiwand Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digiwand Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

brianacnguyen left a comment

Choose a reason for hiding this comment

digiwand commented Jan 30, 2025 •

edited

Loading

After Transaction Review BlockaidBanner positive padding

github-actions bot commented Jan 30, 2025 •

edited by metamaskbot

Loading

jpuri commented Jan 31, 2025 •

edited

Loading

After Transaction Review BlockaidBanner positive padding

digiwand Jan 31, 2025 •

edited

Loading

digiwand Feb 13, 2025 •

edited

Loading