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

Location of critical value of bar scales in code has risk of being overridden in translations #90

Closed
3 tasks done
boisei0 opened this issue Aug 3, 2020 · 4 comments
Closed
3 tasks done

Comments

@boisei0
Copy link

boisei0 commented Aug 3, 2020

The critical value, in the code referenced as both “signaalwaarde” and “kritiekeWaarde”, is sourced in the locale. The default value is present in the Dutch locale. If left empty in translations, the Dutch value is used to override the translation. However, if it erroneously isn’t left empty and the value differs from the original, the interpretation strongly changes.

Should values that act as constants and don’t change upon translating be present in a locale in the first place, rather than be in file with constants? It would also improve the transparency if constants like these are in an easy place to find, rather than having to study the code in order to find them. Furthermore, storing numbers as numerical value rather than as string removes the need for Number(...) calls in the code and reduces errors caused by accidental typos slipping into the text values.

At the moment of writing, the critical value for IC admittances is located at line 102 in the Dutch locale (value 10):

"translation": "10",

and at line 98 in the English locale (value left blank):

Governance

@VWSCoronaDashboard
Copy link
Contributor

@boisei0

Agreed that these numeric values should ideally not live in a locale file!

If you look at an older version of the app, you can see that at some point not only the text but also all other configuration ended up in a single json files. In hindsight you could say that was premature abstraction of logic.

Not only the kritieke waarde (which has been renamed to signaal waarde) was stored there, but also constants like the min and max value and gradients of specific charts.

Long story short, we're planning to remove these constant from configuration files anyway and place them directly in the JSX.

  1. Flatten the configuration file
  2. Use it as an actual dictionary (now in develop!)
  3. and from that point in time start moving away duplicates, configuration values and other crud from the locale files to better places where possible.

@VWSCoronaDashboard
Copy link
Contributor

Configuration values ("signaalwaarde") have been removed from the locale: 48ed6cf. Thanks for reporting.

@boisei0
Copy link
Author

boisei0 commented Aug 6, 2020

This commit introduced a new issue. The “signaalwaarde” is used twice in these components. First in the bar scale, then in the plot below it.
This line will fail with the changes made, the same with the other components.

signaalwaarde={Number(text.signaalwaarde)}

@VWSCoronaDashboard
Copy link
Contributor

VWSCoronaDashboard commented Aug 7, 2020

@boisei0

You are correct!
I've made this PR to resolve that: #108

Thanks again!

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

No branches or pull requests

2 participants