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

[FIX] Updated EditLegacy Component #5116

Merged
merged 5 commits into from
Oct 13, 2022
Merged

[FIX] Updated EditLegacy Component #5116

merged 5 commits into from
Oct 13, 2022

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Oct 12, 2022

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
This fix creates and uses an updated EditLegacy component for legacy transactions in the Send Flow. The default EditLegacy component is reverted to prevent breaking the app for other legacy transactions (approve, approval and swaps).

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
2. What is the improvement/solution?

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses #???

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@blackdevelopa blackdevelopa requested a review from a team as a code owner October 12, 2022 12:39
@github-actions
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.

Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

Could we just replace the old Gas Fee Legacy component and remove the unused code?

Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

app/components/UI/EditGasFeeLegacy/index.js Show resolved Hide resolved
@blackdevelopa blackdevelopa self-assigned this Oct 12, 2022
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board labels Oct 12, 2022
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Great work!! Just a question about the typing, would be better type the useState and the arguments of functions on this new component EditGasFeeLegacyUpdate?

app/components/UI/EditGasFeeLegacyUpdate/index.tsx Outdated Show resolved Hide resolved
@sethkfman sethkfman added team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead release-5.10.0 Issue or pull request that will be included in release 5.10.0 labels Oct 13, 2022
@chrisleewilcox
Copy link
Contributor

I am not able to cancel a Send transaction on goerli network. In the recording you can see I canceled before transaction was successful...
https://recordit.co/mdXbXqRygH

@chrisleewilcox chrisleewilcox merged commit 4595f96 into main Oct 13, 2022
@chrisleewilcox chrisleewilcox deleted the refactor-cleanup branch October 13, 2022 21:44
@chrisleewilcox
Copy link
Contributor

LGTM.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 13, 2022
@sethkfman sethkfman removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-5.10.0 Issue or pull request that will be included in release 5.10.0 team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants