safe-wallet-web icon indicating copy to clipboard operation
safe-wallet-web copied to clipboard
verified publisher icon safe-global

Refactor: unify confirmations in the transaction flow

Open clovisdasilvaneto opened this issue 1 year ago • 11 comments

What it solves

Unify all the Review screens from the tx-flow floder.

How this PR fixes it

It introduces a refactor in the way how we handle the confirmation views on the SignOrExecuteForm component. This PR introduces the first part of the refactor, by creating a central component to handle the presentation of the Review screens. It also makes all modals from the approval owner flow to use the same SettingsChange component (which was part of the ReviewOwner component.

How to test it

1- Go to the add signer flow (in a modal) 2- Go to the confirmation screen.

You will see the new screen shows a loading screen before the confirmation screen, it happens because we had to move some data outside the SignOrExecuteForm component, and these data was asynchronous, so in order to prevent some glitch in the screen, we show the loading component before we have load the necessary data to show in the component.

Screenshots

Checklist

  • [x] I've tested the branch on mobile 📱
  • [ ] I've documented how it affects the analytics (if at all) 📊
  • [x] I've written a unit/e2e test for it (if applicable) 🧑‍💻

clovisdasilvaneto avatar Sep 16 '24 08:09 clovisdasilvaneto

Branch preview

✅ Deploy successful!

Website: https://chore_refactor_transaction_flow--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook: https://chore_refactor_transaction_flow--walletweb.review.5afe.dev/storybook/

github-actions[bot] avatar Sep 16 '24 08:09 github-actions[bot]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: :white_check_mark: success
  • Annotations: 0 total
Report generated by eslint-plus-action

github-actions[bot] avatar Sep 16 '24 08:09 github-actions[bot]

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 1011.89 KB (🟡 +158 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Four Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/apps 48.42 KB (-1 B) 1.04 MB
/apps/custom 40.06 KB (-1 B) 1.03 MB
/balances/nfts 19.2 KB (-1 B) 1.01 MB
/stake 596 B (🟢 -1 B) 1012.47 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

github-actions[bot] avatar Sep 16 '24 08:09 github-actions[bot]

Coverage report

St.:grey_question:
 Category Percentage Covered / Total
🟡 Statements
74.19% (+0.6% 🔼)
 13746/18527
🔴 Branches
52.77% (+1.35% 🔼)
 3397/6437
🔴 Functions
58.04% (+0.89% 🔼)
 2011/3465
🟡 Lines
75.86% (+0.55% 🔼)
 12485/16459
Show new covered files 🐣
St.:grey_question:
 File Statements Branches Functions Lines
🟢
... / SignOrExecuteForm.tsx
 85.54% 78.75% 42.86% 84.81%
🟢
... / index.tsx
 92.31% 61.9% 100% 100%
🟢
... / index.tsx
 95% 85.71% 100% 100%
🟢
... / ChangeSignerSetupWarning.tsx
 100% 100% 100% 100%
🟢
... / useIsMultichainSafe.ts
 90.91% 100% 66.67% 100%
🟢
... / context.ts
 100% 100% 100% 100%
🟢
... / index.tsx
 100% 100% 100% 100%
🟢
... / context.tsx
 100% 100% 100% 100%
🟢
... / index.tsx
 100% 100% 100% 100%
🟢
... / utils.ts
 100% 100% 100% 100%
🟢
... / SignOrExecuteSkeleton.tsx
 100% 100% 100% 100%
🟡
... / index.tsx
 62.16% 33.33% 42.86% 61.11%
🟢
... / mockData.ts
 100% 100% 100% 100%
🟢
... / mockData.ts
 100% 100% 100% 100%
Show files with reduced coverage 🔻
St.:grey_question:
 File Statements Branches Functions Lines
🟡
... / hooks.ts
79.26% (-1.78% 🔻)
68.29% (-4.43% 🔻)
70% (-0.59% 🔻)
79.51% (-1.62% 🔻)
🔴
... / SafeTxProvider.tsx
27.5% (-1.45% 🔻)
 0%
36.36% (-3.64% 🔻)
27.78% (-1.63% 🔻)
🟡 src/pages/_app.tsx
69.33% (-0.4% 🔻)
 42.86% 0%
68.06% (-0.44% 🔻)
🔴
... / index.tsx
30.08% (-4.88% 🔻)
0% (-1.96% 🔻)
0% (-2.63% 🔻)
33.04% (-4.46% 🔻)

Test suite run success

1582 tests passing in 213 suites.

Report generated by 🧪jest coverage report action from 0c1b25e2aed5092e82e3a733229f1f5de944108f

github-actions[bot] avatar Sep 16 '24 08:09 github-actions[bot]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 1 0
Ignored 0 N/A

  • Result: :white_check_mark: success

  • Annotations: 1 total

[warning] react-hooks/exhaustive-deps

verifies the list of dependencies for Hooks like useEffect and similar

Report generated by eslint-plus-action

github-actions[bot] avatar Sep 16 '24 11:09 github-actions[bot]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: :white_check_mark: success
  • Annotations: 0 total
Report generated by eslint-plus-action

github-actions[bot] avatar Sep 17 '24 11:09 github-actions[bot]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: :white_check_mark: success
  • Annotations: 0 total
Report generated by eslint-plus-action

github-actions[bot] avatar Sep 18 '24 07:09 github-actions[bot]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: :white_check_mark: success
  • Annotations: 0 total
Report generated by eslint-plus-action

github-actions[bot] avatar Sep 19 '24 08:09 github-actions[bot]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: :white_check_mark: success
  • Annotations: 0 total
Report generated by eslint-plus-action

github-actions[bot] avatar Sep 19 '24 13:09 github-actions[bot]

any reason we moved approve at the top again ? We moved approve under main information for the purpose some time ago - https://github.com/safe-global/safe-wallet-web/pull/3855 image

liliya-soroka avatar Sep 19 '24 17:09 liliya-soroka

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: :white_check_mark: success
  • Annotations: 0 total
Report generated by eslint-plus-action

github-actions[bot] avatar Sep 20 '24 12:09 github-actions[bot]

Confirmation Screens Refactor

notion-workspace[bot] avatar Sep 26 '24 13:09 notion-workspace[bot]

waiting for https://github.com/safe-global/safe-client-gateway/pull/1973 to be available on staging before continue the ticket testing

liliya-soroka avatar Oct 08 '24 08:10 liliya-soroka

The name of a new owner is not shown in the review step of adding an owner. Same for owner replacement tx

owner addition name

Here's dev for comparison: image

francovenica avatar Oct 10 '24 02:10 francovenica

The tx I tried worked fine. I still would like for the conflicts to be solved so I can re check it quickly, because I've seen some bugs (like the duplicated parameters in swap) that have been fixed already.

francovenica avatar Oct 11 '24 02:10 francovenica

Refactor confirmation screen

notion-workspace[bot] avatar Oct 17 '24 13:10 notion-workspace[bot]

Replace owner: A new owner name is missing from the previous step image

liliya-soroka avatar Oct 25 '24 10:10 liliya-soroka

The issue of the owner name in the adding and replacing was fixed.

New issue: "Add signer" form shows the new owner twice Just try to add a new owner and reach the review page

image

francovenica avatar Oct 31 '24 00:10 francovenica

The issue is fixed: image

LGTM

francovenica avatar Nov 01 '24 01:11 francovenica