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

[IMPROVEMENT] - Apply Test network prefix to token values to help educate users #4530

Merged
merged 96 commits into from
Aug 18, 2022
Merged

[IMPROVEMENT] - Apply Test network prefix to token values to help educate users #4530

merged 96 commits into from
Aug 18, 2022

Conversation

sethkfman
Copy link
Contributor

@sethkfman sethkfman commented Jun 17, 2022 •
edited

Description

This ticket is for UI improvements to the way we display ETH on testnets. We are prefixing the ETH ticker with the specific testnet.

Screenshots/Recordings

Simulator.Screen.Recording.-.iPhone.11.Pro.-.2022-08-01.at.14.03.00.mp4

Checklist

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

@sethkfman sethkfman requested a review from a team as a code owner June 17, 2022 20:43
@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2022 •
edited

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.

10 out of 11 committers have signed the CLA.
✅ @sethkfman
✅ @Fatxx
✅ @georgewrmarshall
✅ @AkshayBhimani
✅ @tommasini
✅ @blackdevelopa
✅ @chrisleewilcox
✅ @bentobox19
✅ @Cal-L
✅ @jpcloureiro
❌ @seth Kaufman

GitHub can't find an account for Seth Kaufman.
You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@sethkfman sethkfman added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jun 17, 2022
@sethkfman sethkfman self-assigned this Jun 17, 2022
andreahaku
andreahaku reviewed Jul 4, 2022
View reviewed changes
Copy link
Member

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

some minor comment mainly related to unnecessary (in my opinion) new styles. Other than that LGTM.

app/components/UI/AssetOverview/index.js Show resolved Hide resolved
app/components/UI/PaymentRequest/index.js Outdated Show resolved Hide resolved
app/components/UI/PaymentRequest/index.js Show resolved Hide resolved
app/components/UI/Tokens/index.js Show resolved Hide resolved
app/components/UI/TransactionEditor/index.js Outdated Show resolved Hide resolved
app/components/UI/TransactionReview/TransactionReviewSummary/index.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
sethkfman and others added 17 commits July 7, 2022 16:23
@sethkfman
PR feedback
9c86657
@sethkfman
merge conflict resoultion
fd062b5
@sethkfman
updated package json
6c8265b
@sethkfman @rickycodes
updated display when requesting payments on test net
1456df6
@sethkfman @rickycodes
updated currency values in gas polling views
c459fe9
@Fatxx @cortisiko @rickycodes
Remove old gas estimation (#3943)
2a24166 
Co-authored-by: Curtis David <Curtis.David7@gmail.com>
@rickycodes
[5.3] Bump Controllers to 29.0.1 (#4258)
dd6a837
@georgewrmarshall @rickycodes
Adding resolutions for security critical packages (#4445)
5a03f0b 
* Adding resolutions for security critical packages

* More resolutions for packages
@georgewrmarshall @rickycodes
Color updates for icons relative to the 1.5 -> 1.6 design tokens upgr…
a910bfd 
…ade (#4415)

* Color updates for icons relative to the 1.5 -> 1.6 design tokens upgrade

* updating snaps
@rickycodes
Remove subtitles from translation files (#4394)
38ae7d9 
* move logic away from subtitle files

* remove secret_phrase_video_subtitle key from json

* convert language to uppercase for title

* add subtitle util

* add types for rn video

* rename util to video

* add tests

* add language to component so it updates on re-render

* use ext in test

* simplify

* use strings for tests

* call from Object.prototype instead
@AkshayBhimani @rickycodes
Resolved spelling mistake (#4438)
ef36969 
Co-authored-by: ricky <ricky.miller@gmail.com>
@georgewrmarshall @rickycodes
Updating instances of Metamask to MetaMask (#4440)
13e9526
@github-actions @rickycodes
5.2.0 (#4341)
e04f5e1
@rickycodes
[FIX] Prompt camera permission (#4429)
dca4f31
@rickycodes
remove secret_phrase_video_subtitle key (#4474)
65ee209
@rickycodes
bring in subtitles from crowdin (#4477)
65b5721
@sethkfman @rickycodes
track down testnet ETH issue for dapps
a3d1f00
@sethkfman
Merge branch 'improvement/test-net-ticker' of github.com:MetaMask/met…
91e1d57 
…amask-mobile into improvement/test-net-ticker
@sethkfman
Copy link
Contributor Author

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

andreahaku
andreahaku approved these changes Aug 2, 2022
View reviewed changes
Copy link
Member

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

Other than the suggestion about the style, LGTM!

@sethkfman sethkfman added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Aug 2, 2022
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Aug 8, 2022
@chrisleewilcox
Copy link
Contributor

chrisleewilcox commented Aug 8, 2022 •
edited

CW1: Blockexplorer scan link on test networks show 'undefined'

Activity > scroll to bottom > link "View full history on undefined"
Token view > scroll to bottomw > link "View full history on undefined"

UPDATE: this was caused by 4780 and a fix will be provided.

@chrisleewilcox
Copy link
Contributor

chrisleewilcox commented Aug 8, 2022 •
edited

CW2: Some screens that still show fiat balance...

  • Wallet view
  • Left nav under account
  • Token view

Based on the original issue the expectation is that we will not show fiat balance or zero for fiat balance on test networks.
Recording of Send flow on Goerli test network.

Recording of Receive flow on Goerli test network.

UPDATE: it was mentioned above for these views that they will show the fiat balance for now. Will address in future.

@sethkfman sethkfman added release-5.7.0 PRs for release 5.7.0 and removed release-5.6.0 PRs for v5.6.0 release labels Aug 9, 2022
@chrisleewilcox chrisleewilcox added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Aug 9, 2022
chrisleewilcox
chrisleewilcox approved these changes Aug 12, 2022
View reviewed changes
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.

LGTM

@chrisleewilcox chrisleewilcox added 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 labels Aug 12, 2022
@sethkfman sethkfman merged commit dcad0a7 into main Aug 18, 2022
@sethkfman sethkfman deleted the improvement/test-net-ticker branch August 18, 2022 21:53
@github-actions github-actions bot locked and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chrisleewilcox chrisleewilcox chrisleewilcox approved these changes

@andreahaku andreahaku andreahaku approved these changes

Assignees

@sethkfman sethkfman

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