search

greeny / SatisfactoryTools

Satisfactory Tools for planning and building the perfect base.
https://www.satisfactorytools.com/
MIT License
268 stars 57 forks source link

Use single method for formatting graph labels; use Intl object and user's browser locale to format label numbers #72

Closed IanKemp closed 3 years ago

IanKemp commented 3 years ago

As per title.

greeny commented 3 years ago

Hi, thanks for the PR.

Number formatting function should be global, currently it's generated here: https://github.com/greeny/SatisfactoryTools/blob/dev/src/Module/AppModule.ts#L412-L423 . However it's not yet used everywhere. The reason for global function is because in the near future I'm planning on adding support for configuring how to format numbers (e.g. number of decimals).

Also last time I checked, Intl was pretty slow compared to other methods of formatting numbers. Not sure how is it now, will have to check.

IanKemp commented 3 years ago

Are you saying this work is useless/redundant then? Or are you suggesting I define and export a global static function that uses the current logic in AppModule#generateNumberFormattingFunction (which I'm happy to do)? My only issue with the latter is that ProductionToolResult only formats to 2 decimals maximum, whereas AppModule#generateNumberFormattingFunction uses up to 5 decimals.

Regarding Intl, there's no reason why (as part of the static function I propose to define) we can't cache the result of Intl.NumberFormat. Logically it will be slower due to doing the locale lookup, but I also think it's more correct.

greeny commented 3 years ago

To be honest, I'm not sure.

Decimals should be consistent across the whole website (so 2 at the moment).

For locale, I intentionally didn't use Intl, as I think having it consistent on screenshots and (future) exports is better than doing i18n on a website that uses english everywhere anyway.

As this will be changed in the future with settings, I'm not sure if there's a need to "waste" time with this PR, making it behave similarly, as there will be bigger change anyway (see #49). If you want to, you can prepare a fix for decimal places from 5 to 2 + reusing that function to format numbers in production tool. However as said previously, it'll be changed "soon" anyway, so not sure if that's worth your time.

IanKemp commented 3 years ago

But when is #49 going to be merged? It's been open since December and was last pushed to in March, but nothing since then. Also it targets the dev branch not u4-dev.

greeny commented 3 years ago

It was opened when u4-dev didn't exist. Also my next goal is merging u4-dev into dev with some code to handle localstorage migration, so eventually we'll get back to having only dev.

Work on #49 is slow, as it's rewriting the whole app, but it's ongoing, a lot of changes are also local without being pushed. I can't really give you any estimation unfortunately. There's also a v2 of API in works, which will also opensource API and also require a lot of frontend changes.

IanKemp commented 3 years ago

Not asking for an estimation, just that long-lived branches that aren't merged generally worry me. :) You seem to have a plan, so I'll leave you to it, but if there's anything I can help with let me know - I'm a C#/TS/JS dev but can pick up Angular if necessary.

greeny commented 3 years ago

well currently I'm thinking of reusing parts of the code in the #49 branch for the v2, which would start from scratch instead of trying to rewrite existing app, that way we could start testing stuff much earlier and wouldn't have to be complete switch. There's also the problem that I currently have very limited time due to a few other projects I'm working on. I'm hoping that in following weeks I'll have more time and start moving this closer to v2.