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.

Sign up for GitHub

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

Jump to bottom

chore: Custom Gas Modal Component #6287

Merged
merged 24 commits into from
Jul 26, 2023
Merged

chore: Custom Gas Modal Component #6287

merged 24 commits into from
Jul 26, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Apr 27, 2023 •
edited
Loading

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 PR will introduce a custom gas modal component that should handle the different transaction type for editing gas fees. It's a code improvement PR and so there should be no visible changes to the app UX or UI. This PR will cover the confirm screen for send flow.

Test Case:

Screenshots/Recordings
http://recordit.co/1yUAJhqhOy

E2E: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4b890180-5a54-4361-bc1d-955a03d41f5d

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 April 27, 2023 14:06
@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.

@blackdevelopa blackdevelopa marked this pull request as draft April 27, 2023 14:07
@blackdevelopa blackdevelopa marked this pull request as ready for review April 28, 2023 11:40
@blackdevelopa blackdevelopa self-assigned this Apr 28, 2023
@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-confirmations-secure-ux-PR PR from the confirmations team labels Apr 28, 2023
@gauthierpetetin gauthierpetetin added in-progress and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 28, 2023
@blackdevelopa blackdevelopa mentioned this pull request Apr 28, 2023
Closed
3 tasks
@bschorchit
Copy link

Hey @gauthierpetetin, did you remove the needs-dev-review and added a in-progress for this PR by mistake? We currently use the needs-dev-review label for internal processes in the team. cc: @hilvmason

@gauthierpetetin
Copy link
Contributor

Hi @bschorchit , we setup an automation yesterday to update the label of the Github issue every time the corresponding Zenhub issue is moved to a new column (more detail here). In this case the ticket was moved from "needs-dev-review" to "in-progress" column.

However, I don't understand why it impacts your board, because the automation is only meant to work for the MetaMask Mobile board. I'll investigate and come back to you, sorry for that.

@gauthierpetetin gauthierpetetin added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed in-progress labels Apr 28, 2023
@gauthierpetetin
Copy link
Contributor

@bschorchit we've just added an additional filter to make sure the automation only applies to issues with the label "team-mobile-client". It shall not happen again on your board. In case it does, don't hesitate to reach out.

@bschorchit
Copy link

No worries, thank you for the context and quick fix, @gauthierpetetin !

@jpuri
Copy link
Contributor

jpuri commented May 1, 2023 •
edited
Loading

@blackdevelopa : while testing with legacy transaction from test DAPP I found that it is showing advance options for legacy transaction:

Screen.Recording.2023-05-01.at.2.28.30.PM.mov

@blackdevelopa
Copy link
Contributor Author

blackdevelopa commented May 3, 2023 •
edited
Loading

@jpuri thanks for flagging this. It exists in production and a new issue should be created for it.

@blackdevelopa : while testing with legacy transaction from test DAPP I found that it is showing advance options for legacy transaction:

Screen.Recording.2023-05-01.at.2.28.30.PM.mov

segun
segun reviewed May 3, 2023
View reviewed changes
Copy link
Contributor

@segun segun left a comment

Choose a reason for hiding this comment

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

Just one comment.

jpuri
jpuri requested changes May 3, 2023
View reviewed changes
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.

Hey, can you plz get rid of nested component declaration. PR looks good otherwise.

segun
segun previously approved these changes May 5, 2023
View reviewed changes
segun
segun previously approved these changes May 16, 2023
View reviewed changes
@blackdevelopa blackdevelopa requested a review from jpuri May 16, 2023 18:22
@blackdevelopa blackdevelopa changed the title Custom Gas Modal Component chore: Custom Gas Modal Component Jul 24, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

78.9% 78.9% Coverage
0.3% 0.3% Duplication

@blackdevelopa blackdevelopa removed the request for review from cryptotavares July 26, 2023 16:45
@blackdevelopa
Copy link
Contributor Author

Merging this PR as it is now QA passed

@blackdevelopa blackdevelopa merged commit 2c462b1 into main Jul 26, 2023
@blackdevelopa blackdevelopa deleted the custom_gas_modal branch July 26, 2023 16:48
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2023
@metamaskbot metamaskbot added the release-7.5.0 Issue or pull request that will be included in release 7.5.0 label Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cryptotavares cryptotavares cryptotavares left review comments

@jpuri jpuri jpuri approved these changes

@segun segun segun approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels
QA Passed A successful QA run through has been done release-7.5.0 Issue or pull request that will be included in release 7.5.0 team-confirmations-secure-ux-PR PR from the confirmations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@blackdevelopa @bschorchit @gauthierpetetin @jpuri @seaona @segun @cryptotavares @metamaskbot