feat: finish up UI changes and code styling #24429

BZahory · 2024-05-08T10:46:46Z

Description

Related issues

Fixes:

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask 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.

github-actions · 2024-05-08T10:46:57Z

CLA Signature Action:

Thank you for your submission, we really appreciate it. We ask that you all read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository.

1 out of 2 committers have signed the CLA.
✅ @BZahory
❌ @ejwessel

micaelae · 2024-05-09T17:52:29Z

ui/ducks/send/send.js

+    if (isSwapAndSend) {
+      dispatch(
+        actions.addNewDraft({
+          ...draftTransactionInitialState,
+          ...state[name].prevSwapAndSendInput,
+          id: transactionId,
+          fromAccount: account,
+          history: [
+            `sendFlow - user clicked edit on transaction with id ${transactionId} (swap and send)`,
+          ],
+        }),
+      );
+      dispatch(updateSendQuote());
+    } else if (assetType === AssetType.native) {


Did you consider decoding the txData instead to extract the draft transaction details? The decoder can also be used to populate the recipient in the confirmation page (we currently show the swaps contract address)

The swaps-recipient bug was a different that's since been resolved. However, you're 100% right about decoding being preferred. I realize that we lose the prevSwapAndSendInput whenever the extension is closed so that edge makes this worth implementing.

Side note but it always amazes me how well you can pick up on what's going on in code – no doubt a huge contributor to the quality of your reviews. You're by far the best reviewer I've come across and I really do appreciate that ❤️

update flexbox stylings

aggregate state vars

broken from rebase

removes a console error

ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx

## **Description**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24535?quickstart=1) ## **Related issues** Fixes: #24491 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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. --------- Signed-off-by: Ethan Wessel <ethan.wessel@consensys.net>

…ap+send state (#24568)  ## **Description**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24568?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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.

## **Description**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24570?quickstart=1) ## **Related issues** Fixes: #24464 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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. --------- Signed-off-by: Ethan Wessel <ethan.wessel@consensys.net>

blocks additional submissions

micaelae · 2024-05-17T18:33:38Z

ui/ducks/send/send.js

@@ -2090,6 +2090,10 @@ const debouncedComputeEstimatedGasLimit = debounce((dispatch) => {
  dispatch(computeEstimatedGasLimit());
 }, 300);

+const debouncedAddHistoryEntry = debounce((dispatch, payload) => {
+  dispatch(addHistoryEntry(payload));
+}, 100);


what's the reasoning behind 100? what's the benefit of debouncing?

Prevents the history array from getting blown up when the user types (e.g., 123 -> 1, 12, 123)

ui/ducks/send/send.js

micaelae · 2024-05-17T18:48:24Z

ui/components/multichain/pages/send/send.js

+    setIsSubmitting(true);
+    await dispatch(signTransaction(history));


Does the page fail gracefully if signing a swap + send transaction/approval fails? Example concerns: page getting stuck in loading state, user doesn't get feedback about the failed signing

It did not – updated

ui/ducks/send/send.js

there is no confirmation screen so this adds clarity

Co-authored-by: micaelae <100321200+micaelae@users.noreply.github.com>

BZahory requested a review from a team as a code owner May 8, 2024 10:46

Base automatically changed from mb843/updates to mb843/swap-and-send May 8, 2024 17:50

micaelae requested review from a team as code owners May 8, 2024 17:50

BZahory force-pushed the mb843/updates-2 branch from b140e19 to 120e2cb Compare May 9, 2024 16:09

micaelae reviewed May 9, 2024

View reviewed changes

BZahory force-pushed the mb843/swap-and-send branch from fbf3ac0 to a3fa6a7 Compare May 10, 2024 15:29

BZahory requested a review from a team as a code owner May 10, 2024 15:29

BZahory added 21 commits May 10, 2024 11:30

chore: create util for swap+send quotes

39a3c81

fix: clear quote data when switching to send

d6ee965

chore: merge files after rebase

f958343

chore: prevent scroll wiggle

9945784

update flexbox stylings

fix: only reset gas limit if no swap

8cfc3b1

fix: prevent infinite useeffect loop

bfd1a13

aggregate state vars

chore: remove no conversion rate text

af7a240

chore: prevent special symbols in input

84d46ed

chore: tweak icon size

42fa82f

feat: preserve swap+send form on edit

db70b04

fix: update variable name

7207445

broken from rebase

fix: reset amount when sender changes

9f74b17

fix: prevent console error for undefined

548ab84

chore: remove p > div nesting

60ad5fb

removes a console error

chore: use statically checkable deps

6552b66

docs: update task list

b0768c3

chore: only show TOS when quote is fetched

7235499

perf: reuse formatted value

1b48bf3

perf: reorder dispatches

7af6b7f

chore: avoid deconstruction

dd85478

perf: reduce action holdup

9624dd5

micaelae reviewed May 15, 2024

View reviewed changes

ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx Outdated Show resolved Hide resolved

micaelae reviewed May 15, 2024

View reviewed changes

ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx Outdated Show resolved Hide resolved

micaelae reviewed May 15, 2024

View reviewed changes

ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx Outdated Show resolved Hide resolved

BZahory and others added 15 commits May 15, 2024 17:08

feat: focus input on key press

8a476ec

chore: remove unused variables

7035c52

style: align to lint and import rules

60e4dda

docs: add comment for out of scope bug

c98fe7b

fix: validate on amount error state change

ccd59ec

feat: submit both transactions on confirm

6f6463a

chore: route to tx screen for swap+send

387d4f8

feat: add loading state

25d5774

blocks additional submissions

perf: debounce amount history entries

8a41461

fix: ignore casing for address equality

5b27394

chore: use cleanup for state change

68eecba

chore: go to activity tab after submitting

2d91f05

micaelae reviewed May 17, 2024

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

micaelae reviewed May 17, 2024

View reviewed changes

ui/ducks/send/send.js Outdated Show resolved Hide resolved

BZahory and others added 8 commits May 19, 2024 23:41

fix: prevent input overflow-push on firefox

941f5ae

fix: hide max button when native send impossible

c091dd2

chore: show "confirm" for swap+send

c72c615

there is no confirmation screen so this adds clarity

fix: prevent infinite loading state

6075ead

chore: format modal balances

5396edf

fix: only reset on impossible gas if max

a14eff7

refactor: get decimals from object

ad8a602

chore: remove redundant toLowerCase()

95ffcf6

Co-authored-by: micaelae <100321200+micaelae@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: finish up UI changes and code styling #24429

feat: finish up UI changes and code styling #24429

BZahory commented May 8, 2024 •

edited by github-actions bot

github-actions bot commented May 8, 2024 •

edited

micaelae May 9, 2024

BZahory May 10, 2024

BZahory May 10, 2024

micaelae May 17, 2024

BZahory May 20, 2024

micaelae May 17, 2024

BZahory May 20, 2024

		setIsSubmitting(true);
		await dispatch(signTransaction(history));

feat: finish up UI changes and code styling #24429

Are you sure you want to change the base?

feat: finish up UI changes and code styling #24429

Conversation

BZahory commented May 8, 2024 • edited by github-actions bot

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented May 8, 2024 • edited

micaelae May 9, 2024

Choose a reason for hiding this comment

BZahory May 10, 2024

Choose a reason for hiding this comment

BZahory May 10, 2024

Choose a reason for hiding this comment

micaelae May 17, 2024

Choose a reason for hiding this comment

BZahory May 20, 2024

Choose a reason for hiding this comment

micaelae May 17, 2024

Choose a reason for hiding this comment

BZahory May 20, 2024

Choose a reason for hiding this comment

BZahory commented May 8, 2024 •

edited by github-actions bot

github-actions bot commented May 8, 2024 •

edited