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

fix: fix coin-overview display when price setting is off #28569

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Nov 20, 2024

Description

Fixes display bug on coin overview and account list item when user toggles OFF the Show balance and token price checker setting

Open in GitHub Codespaces

Related issues

Fixes: #28567

Manual testing steps

  1. Go to settings turn off Show balance and token price checker
  2. Go back to home page you should see correct balance in crypto
  3. Click on account item menu and you should see balance in crypto
  4. Go to settings and turn it ON and you should see correct fiat balance.
  5. Go to settings and turn on the "show native token as main balance"
  6. You should see balance in crypto
  7. Go to settings and turn OFF "show native token as main balance" also turn OFF "Show balance and token price checker"
  8. You should see balance in crypto

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

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.

@metamaskbot
Copy link
Collaborator

Builds ready [6ee9cec]
Page Load Metrics (2153 ± 108 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint187327492145223107
domContentLoaded185926662109213102
load187527522153225108
domInteractive28267545225
backgroundConnect1084382613
firstReactRender5511587178
getState5110779209
initialActions01000
loadScripts13632059155716680
setupStore6451084
uiStartup217831312442232111
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 217 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@bergeron bergeron left a comment

Choose a reason for hiding this comment

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

LGTM

before:

Screenshot 2024-11-20 at 10 43 54 AM

after:

Screenshot 2024-11-20 at 10 44 21 AM

@sahar-fehri sahar-fehri marked this pull request as ready for review November 20, 2024 21:51
@sahar-fehri sahar-fehri requested review from a team as code owners November 20, 2024 21:51
@sahar-fehri sahar-fehri added this pull request to the merge queue Nov 20, 2024
Merged via the queue into develop with commit f270621 Nov 20, 2024
84 checks passed
@sahar-fehri sahar-fehri deleted the fix/fix-display-with-price-setting branch November 20, 2024 22:28
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 20, 2024
@seaona
Copy link
Contributor

seaona commented Nov 21, 2024

Thank you for the fix, I also see it working on my end 🙏 One suggestion: I think we should add an e2e test that covers this flow, so this bug is not re-introduced in the future. What do you think?

fix-balance-fiat.mp4

@sahar-fehri
Copy link
Contributor Author

Totally! Added an e2e on a followup fix here #28598 🙏

@seaona
Copy link
Contributor

seaona commented Nov 21, 2024

awesome, thank you @sahar-fehri ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Incorrect Balance - When I have Price API toggle disabled, the balance displayed is incorrect
5 participants