-
Notifications
You must be signed in to change notification settings - Fork 191
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
fix: handle undefined options in number abbrev #3860
Conversation
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 |
@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 |
@DiggyDiggy2 @reallybeard are ok with this conceptually, let's not block on @pastaghost |
Agreed. AssetService is going to take a day or two as I'm trying to button up the KeepKey work right now. |
@0xdef1cafe |
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. |
There was a problem hiding this 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 🐐
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 byundefined
.The "10" default forThis was used because later the number is truncated instead of rounded, I didn't understand this at first.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.Notice
Pull Request Type
Issue (if applicable)
closes #3854
Risk
Modifies the logic for fiat amounts formatting across the whole app.
Testing
Engineering
Operations
Screenshots (if applicable)
Price Chart tooltip after the fix:
2023-02-17_11-41-18.webm