Skip to content

Allow custom network fiat values#2863

Merged
wachunei merged 7 commits into
developfrom
feature/custom-network-fiat-values
Jul 26, 2021
Merged

Allow custom network fiat values#2863
wachunei merged 7 commits into
developfrom
feature/custom-network-fiat-values

Conversation

@wachunei
Copy link
Copy Markdown
Member

@wachunei wachunei commented Jul 1, 2021 •
edited
Loading

Description

  • Allow showing fiat values when using custom networks
  • This PR uses @metamask/controllers v12

Screen Shot 2021-07-01 at 10 41 13Screen Shot 2021-07-01 at 10 40 55Screen Shot 2021-07-01 at 10 40 27

Issue

Resolves #2440
Fixes #1929

@wachunei wachunei mentioned this pull request Jul 1, 2021
Merged
sethkfman
sethkfman reviewed Jul 2, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Will you be needing to update any test cases?

@wachunei
Copy link
Copy Markdown
Member Author

wachunei commented Jul 2, 2021

Will you be needing to update any test cases?

Maybe snapshots, not sure

@wachunei wachunei marked this pull request as ready for review July 7, 2021 19:46
@wachunei wachunei requested a review from a team as a code owner July 7, 2021 19:46
@wachunei wachunei added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 7, 2021
@wachunei wachunei force-pushed the feature/custom-network-fiat-values branch from 5c1d149 to 5340e80 Compare July 14, 2021 19:29
Cal-L
Cal-L requested changes Jul 14, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Cal-L Cal-L left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@wachunei
Code LGTM. One thing that we should do though (and probably more standard) is to display at least 2 trailing numbers after the decimal for USD. Not sure about other currencies, but I suspect the case for those.

@wachunei
Copy link
Copy Markdown
Member Author

wachunei commented Jul 14, 2021

@Cal-L Yeah, we are aware of this issue and there's a proposal we have started a long time ago but have not finished. https://www.notion.so/Mobile-Numbers-a2cfe7926f754eb8a55e4ac356c52b44

@Cal-L
Copy link
Copy Markdown
Contributor

Cal-L commented Jul 14, 2021

@wachunei
Sounds good, yeah if we're going to handle that as a separate issue, then this PR LGTM. 🚀

@wachunei wachunei removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 14, 2021
@wachunei wachunei self-assigned this Jul 15, 2021
@wachunei
Copy link
Copy Markdown
Member Author

wachunei commented Jul 26, 2021

@ibrahimtaveras00 We are merging this. QA needed on develop only to spotcheck the balances showing up back again.

@wachunei wachunei merged commit 72d6904 into develop Jul 26, 2021
@wachunei wachunei deleted the feature/custom-network-fiat-values branch July 26, 2021 21:37
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
@wachunei
Allow custom network fiat values (#2863)
3689f7f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Cal-L Cal-L Cal-L approved these changes

+1 more reviewer

@sethkfman sethkfman sethkfman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wachunei wachunei

Labels

needs-qa Any New Features that needs a full manual QA prior to being added to a release.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

View the FIAT conversion of my tokens Disable fiat values when user is connected to a custom or testnet RPC

3 participants

@wachunei @Cal-L @sethkfman