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: handle undefined options in number abbrev #3860

Merged
merged 5 commits into from
Feb 23, 2023

Conversation

firebomb1
Copy link
Contributor

@firebomb1 firebomb1 commented Feb 17, 2023

Description

Handling of options which have an unidentified value when abbreviating numbers (e.g. coming from components with optional props without default values) in order for them to not override the defaults set based on the number being formatted.

This fixes a bug which was visible in the Price Chart tooltip, the number of fractional digits for low value assets was not dynamic and always set at the default value of Intl.NumberFormat for currencies (2) because the default was always overridden by undefined.

The "10" default for maximumFractionDigits mentioned above is also removed to the benefit of the dynamic computation that was already done but not used either for a reason I don't know. This was used because later the number is truncated instead of rounded, I didn't understand this at first.

Notice

  • Have you followed the guidelines in our Contributing guide?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #3854

Risk

Modifies the logic for fiat amounts formatting across the whole app.

Testing

Engineering

Operations

  • Verify that the Price Chart tooltip works for low/high fiat value assets and display more precise numbers for low value assets.
  • Verify that amounts in fiat displayed across the app still respect the format we previously used.

Screenshots (if applicable)

Price Chart tooltip after the fix:

2023-02-17_11-41-18.webm

@firebomb1 firebomb1 self-assigned this Feb 17, 2023
@firebomb1
Copy link
Contributor Author

This is a draft until Product gives their OK for altering the current values displayed by the Price Chart tooltip, I've asked them on Discord and will update when there is an answer.

If the values must remain imprecise for some reason, I still think the underlying bug should be fixed, but then we'll need to set a "2" for the maximumFractionDigits of that tooltip usage of <Amount.Fiat>.

@firebomb1 firebomb1 added the needs product Requires product input before bounty label Feb 20, 2023
@0xdef1cafe
Copy link
Collaborator

@firebomb1 you may get stomped on by the large @pastaghost here - he's got a very nice suggestion w.r.t how we handle asset values that may or may not capture this - have a chat to him

@0xdef1cafe
Copy link
Collaborator

@DiggyDiggy2 @reallybeard are ok with this conceptually, let's not block on @pastaghost AssetValue changes as those are a ways out

@0xdef1cafe 0xdef1cafe marked this pull request as ready for review February 22, 2023 00:01
@0xdef1cafe 0xdef1cafe requested a review from a team as a code owner February 22, 2023 00:01
@pastaghost
Copy link
Collaborator

@DiggyDiggy2 @reallybeard are ok with this conceptually, let's not block on @pastaghost AssetValue changes as those are a ways out

Agreed. AssetService is going to take a day or two as I'm trying to button up the KeepKey work right now.

@firebomb1 firebomb1 removed the needs product Requires product input before bounty label Feb 22, 2023
@pastaghost
Copy link
Collaborator

pastaghost commented Feb 22, 2023

@0xdef1cafe
Update: I've got a PR open for AssetValue here: shapeshift/lib#1206

@firebomb1
Copy link
Contributor Author

firebomb1 commented Feb 22, 2023

Thanks for the link, the new lib will deal with crypto value handling/formatting and the current issue/PR relate to fiat value formatting, so while complementary at some point (to get a fiat balance) they should not impact each other as far as I can see.

gomesalexandre
gomesalexandre previously approved these changes Feb 23, 2023
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested locally, LGTM! Great PR once again @firebomb1 🐐

@gomesalexandre gomesalexandre merged commit 9d1b22c into shapeshift:develop Feb 23, 2023
@firebomb1 firebomb1 deleted the fix-price-chart-digits branch February 24, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Price chart tooltip not displaying enough fractional digits for low fiat value assets
4 participants