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

Implement warning for deprecated test networks, kovan, ropsten and rinkeby #4885

Merged
merged 13 commits into from
Aug 24, 2022
Merged

Implement warning for deprecated test networks, kovan, ropsten and rinkeby #4885

merged 13 commits into from
Aug 24, 2022

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Aug 18, 2022 •
edited

Description
Kovan, Ropsten and Rinkeby are 3 test networks that will be deprecated soon with the merge of ethereum.

Proposed Solution
Created a warning with the information when the user change for one of this three networks.

Screenshots/Recordings
https://recordit.co/WeoxoIWOoo

Issue

Progresses #4697

Checklist

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

@tommasini tommasini marked this pull request as ready for review August 18, 2022 17:40
@tommasini tommasini requested a review from a team as a code owner August 18, 2022 17:40
@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.

@tommasini tommasini added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) release-5.7.0 PRs for release 5.7.0 labels Aug 18, 2022
@bschorchit
Copy link

This looks great! Thank you, @tommasini

@bschorchit bschorchit mentioned this pull request Aug 18, 2022
Closed
4 tasks
sethkfman
sethkfman approved these changes Aug 23, 2022
View reviewed changes
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

cortisiko
cortisiko requested changes Aug 23, 2022
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

@tommasini found a couple 🐛

In dark mode: there is a dark artifact in the warning alert https://www.screencast.com/t/42EScJP1ofhd

In light mode: I cannot read the text in the warning alert:
https://www.screencast.com/t/VuL2JAB0A

Usability issue:
If I create a new wallet and I switch to a deprecated test net I see two warning alerts: The protect your wallet and network deprecation. Which one takes precedence? https://www.screencast.com/t/hbGAhZHyRS

Furthermore, If I were to open the burger menu I am unable to see the menu options to scroll through: http://recordit.co/ot71GMZndf

@cortisiko cortisiko added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Aug 23, 2022
@cortisiko cortisiko added QA in Progress QA has started on the feature. QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA in Progress QA has started on the feature. labels Aug 23, 2022
cortisiko
cortisiko approved these changes Aug 23, 2022
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

🌮 🌮 🌮

@tommasini tommasini merged commit 41d02e3 into main Aug 24, 2022
@tommasini tommasini deleted the implement/4697-deprecated-testnetworks branch August 24, 2022 13:19
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2022
@omnat
Copy link
Contributor

omnat commented Aug 25, 2022

Is this shown just once per user or everytime they switch to one of the 3 networks @tommasini ?

@tommasini
Copy link
Contributor Author

tommasini commented Aug 25, 2022 •
edited

The behaviour right now is:

  • Everytime they switch the network to one of the three the warning will prompt
  • If they close the alert it will only appear when they kill the app and re open it
  • It will only shows on the 3 test networks, users that don't switch to them will not see the alert

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sethkfman sethkfman sethkfman approved these changes

@cortisiko cortisiko cortisiko approved these changes

Assignees
No one assigned
Labels
QA Passed A successful QA run through has been done release-5.7.0 PRs for release 5.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tommasini @bschorchit @omnat @sethkfman @cortisiko