Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Replace redesign confirmation BottomModal with BottomSheet #13268

Merged
merged 46 commits into from
Feb 28, 2025

Conversation

digiwand
Copy link
Contributor

@digiwand digiwand commented Jan 30, 2025

Description

Updates:

  • Replaces BottomModal with BottomSheet in redesign Confirm page
    • Remove min height 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
  • Update BlockaidBanner to remove surrounding margin to allow for better reusability
    • Replace BlockaidBanner instances margin overrides with surrounding container margins
  • Update BottomSheet to pass style to BottomSheetDialog sheet cc: @brianacnguyen
    • 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. Instead of making this change, we will continue to update the bottomsheet, footer, and header when applicable

Fixes:

  • 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

CleanShot 2025-02-09 at 01 35 46@2x
CleanShot 2025-02-09 at 01 35 54@2x

After Confirm.test no console.errors

CleanShot 2025-02-09 at 01 49 47@2x

Pre-merge author checklist

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
Copy link
Contributor

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.

@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Jan 30, 2025
@digiwand digiwand marked this pull request as ready for review January 30, 2025 18:22
@digiwand digiwand requested review from a team as code owners January 30, 2025 18:22
@digiwand digiwand added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jan 30, 2025
Copy link
Contributor

github-actions bot commented Jan 30, 2025

https://bitrise.io/ 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

@jpuri
Copy link
Contributor

jpuri commented Jan 31, 2025

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

Screenshot 2025-01-31 at 4 12 25 PM

Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

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

adding styles overwrites alignment applied styles (flex: 1)
also allow inherit disabled styles to be used
digiwand and others added 3 commits February 25, 2025 11:45
removing this option seems to allow background to fade in naturally
if this doesn't work try animation: 'fade'
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.46%. Comparing base (256bc97) to head (a7c539a).
Report is 45 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13268      +/-   ##
==========================================
+ Coverage   62.91%   63.46%   +0.55%     
==========================================
  Files        2035     2095      +60     
  Lines       44539    45301     +762     
  Branches     6050     6236     +186     
==========================================
+ Hits        28020    28750     +730     
+ Misses      14685    14670      -15     
- Partials     1834     1881      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@digiwand digiwand requested review from OGPoyraz and jpuri February 28, 2025 06:36
jpuri
jpuri previously approved these changes Feb 28, 2025
@digiwand digiwand added this pull request to the merge queue Feb 28, 2025
Merged via the queue into main with commit a13b9af Feb 28, 2025
38 of 39 checks passed
@digiwand digiwand deleted the refactor-signature-use-bottom-sheet branch February 28, 2025 19:01
@github-actions github-actions bot locked and limited conversation to collaborators Feb 28, 2025
@metamaskbot metamaskbot added the release-7.43.0 Issue or pull request that will be included in release 7.43.0 label Feb 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.43.0 Issue or pull request that will be included in release 7.43.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: Replace BottomModal with BottomSheet in redesign confirmation
6 participants