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 #222 simple balance crash #244

Merged
merged 3 commits into from
Aug 7, 2019
Merged

Conversation

MaxStalker
Copy link
Member

Fixes #222
@TimDaub users who had esperiences crash before now should be able to access their funds :)

@MaxStalker MaxStalker requested a review from vrde August 6, 2019 22:58
Copy link
Collaborator

@TimDaub TimDaub left a comment

Choose a reason for hiding this comment

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

Screen Shot 2019-08-07 at 13 10 36

Thanks Max! This is a great bug fix!
For posterity, what exactly crashed Safari now? Was it formatter.formatToParts(amount)?
Because if so, we should file a bug on i18next.

@MaxStalker
Copy link
Member Author

MaxStalker commented Aug 7, 2019

For posterity, what exactly crashed Safari now? Was it formatter.formatToParts(amount)?
Because if so, we should file a bug on i18next.

Yap, but it's not i18next issue. That's simply browser implementation of Intl.NumberFormat:
https://developer.mozilla.org/ru/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat

And, my guess, is that iOS Safari is not implementing .formatToParts method.

@MaxStalker MaxStalker merged commit bdfd3fc into master Aug 7, 2019
@MaxStalker MaxStalker deleted the fix/222-simple-balance-crash branch August 7, 2019 11:20
Copy link
Collaborator

@vrde vrde left a comment

Choose a reason for hiding this comment

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

Sorry for reviewing this after it has been merged. About my comment, we can talk here and decide if we want to address it in another PR.

@@ -20,6 +20,12 @@
integrity="sha384-B4dIYHKNBt8Bc12p+WXckhzcICo0wtJAoU8YZTY5qE0Id1GSseTk6S+L3BlXeVIU"
crossorigin="anonymous"
/>

<link
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this belongs to this PR. Also, I'd serve the font from the burner wallet domain itself, to avoid sharing information with the big G.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, that adding font should have been gone into another issue 🙇‍♂️

But can you tell us what is the concerns? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, what I would like to avoid is loading assets from Google's servers because I'm concerned about privacy. The less data points we give to Google, the better, so I would prefer if we can download the resource and serve it from "our servers".

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.

Simple Balance doesn't render properly on Safari Mobile
3 participants