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: custom network component #7159

Merged
merged 6 commits into from
Sep 28, 2023
Merged

Conversation

wachunei
Copy link
Member

@wachunei wachunei commented Sep 6, 2023

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 the appropiate QA label when dev review is completed
    • needs-qa: PR requires manual QA.
    • No QA/E2E only: PR does not require any manual QA effort. Prior to merging, ensure that you have successful end-to-end test runs in Bitrise.
    • Spot check on release build: PR does not require feature QA but needs non-automated verification. In the description section, provide test scenarios. Add screenshots, and or recordings of what was tested.
  5. Add QA Passed label when QA has signed off (Only required if the PR was labeled with needs-qa)
  6. Add your team's label, i.e. label starting with team- (or external-contributor label if your not a MetaMask employee)

Description

This PR refactors the CustomNetwork component to include it in #7169 feat(ramp): add NetworkSwitcher view

Changes
Add

  • Optional prop onNetworkSwitch that overrides default navigation behavior
  • Optional prop showAddedNetworks to show currently added network, changing the action from "Add" to "Switch" network
  • Optional prop customNetworksList that overrides the current PopularList variable being read inside the component
  • "Switch" string

Change

  • Change ImageIcon component to a AvatarNetwork component that will take imageSource or { uri: imageUrl }, defaulting to the circled network initial letter in case none is present
  • Hide the warning icon if toggleWarningModal is not passed as prop
  • Refactor Network interface to add rpcPrefs.imageUrl as optional property
  • Remove CustomNetworkProps interface extension of Networks

QA
This PR must not change current only reference of this component

<CustomNetwork
isNetworkModalVisible={this.state.showPopularNetworkModal}
closeNetworkModal={this.onCancel}
selectedNetwork={this.state.popularNetwork}
toggleWarningModal={this.toggleWarningModal}
showNetworkModal={this.showNetworkModal}
switchTab={this.tabView}
shouldNetworkSwitchPopToWallet={
shouldNetworkSwitchPopToWallet
}
/>

Screenshots/Recordings

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

Issue

fixes #???

Checklist

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

@wachunei wachunei requested a review from a team as a code owner September 6, 2023 22:52
@wachunei wachunei added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-platform labels Sep 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (2f339b7) 34.60% compared to head (3c2fe0c) 34.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7159      +/-   ##
==========================================
- Coverage   34.60%   34.59%   -0.01%     
==========================================
  Files        1017     1017              
  Lines       27144    27150       +6     
  Branches     2205     2211       +6     
==========================================
  Hits         9393     9393              
- Misses      17262    17268       +6     
  Partials      489      489              
Files Coverage Δ
app/components/UI/NetworkModal/index.tsx 4.34% <0.00%> (-0.20%) ⬇️
...etworkSettings/CustomNetworkView/CustomNetwork.tsx 4.54% <0.00%> (-1.02%) ⬇️

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

Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

@wachunei please run the smoketests against this branch on bitrise. Let us know if you need assistance on doing that.

@wachunei
Copy link
Member Author

@wachunei
Copy link
Member Author

@chrisleewilcox should I re run smoke tests?

@wachunei
Copy link
Member Author

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.

Awesome! LGTM!

@wachunei wachunei dismissed chrisleewilcox’s stale review September 28, 2023 17:03

Passing smoke tests were requested: app.bitrise.io/app/be69d4368ee7e86d/pipelines/c9f35e14-4f47-4a38-9c0a-f677162d58ca

@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 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@wachunei wachunei merged commit 047ec00 into main Sep 28, 2023
@wachunei wachunei deleted the refactor/custom-networks-component branch September 28, 2023 17:46
@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2023
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 28, 2023
@metamaskbot metamaskbot added the release-7.9.0 Issue or pull request that will be included in release 7.9.0 label Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.9.0 Issue or pull request that will be included in release 7.9.0 team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants