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

CurrencyFormatter is not efficient #187

Closed
p0o opened this issue Mar 1, 2016 · 4 comments
Closed

CurrencyFormatter is not efficient #187

p0o opened this issue Mar 1, 2016 · 4 comments

Comments

@p0o
Copy link
Contributor

p0o commented Mar 1, 2016

I'm not happy with how currency formatter is adding this fat 1400 lines of unnecessary codes to our codebase. Maybe we can remove all the unwanted currencies from this module and use it as an internal one?

@gpbl
Copy link
Contributor

gpbl commented Mar 1, 2016

This currency formatter is not the best solution as we will get rid of it once we go i18n (#11) – however that specific file, compressed, is less than 2KB, so not that big deal. We should get rid of material-ui first, as it weight 1/3 of our bundle :P

@p0o
Copy link
Contributor Author

p0o commented Mar 1, 2016

But it could easily be simplified in a couple of lines of code. I'm not suggesting to get rid of it, I'm saying we can remove unnecessary currencies from the above file and import it internally. If you agree I can do it in my spare time?

@gpbl
Copy link
Contributor

gpbl commented Mar 1, 2016

I can't see the reason of changing it. I see only disadvantages: we wouldn't be able to update the dependency if the module is updated, we should add tests, we would need to take double care once we switch to react-intl, we need to refactor the imports, etc. all this for saving few bytes seems too much 😄

@p0o
Copy link
Contributor Author

p0o commented Mar 1, 2016

Well, I'm not sure if it's a module with update potential or the situation is as bad as you describe it 😣 but okay if you think so. Just didn't feel good about all those lines. I am closing this issue.

@p0o p0o closed this as completed Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants