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

Set ticks on y axis to -£100 instead of £-100 #820

Merged
merged 5 commits into from
Oct 27, 2023

Conversation

abhcs
Copy link
Collaborator

@abhcs abhcs commented Oct 27, 2023

Description

Fixes #148.

Strategy for fix

The Plotly locale en does not have the correct currency symbol "£". Ideally, Plotly should manage locales (in particular, currency) properly. However, since PolicyEngine only supports two locales, I have re-registered the two locales with the correct currency symbols in PolicyEngine.

Tests

I have tested four policy outputs (AverageImpactByDecile, AverageImpactByWealthDecile, BudgetaryImpact, DetailedBudgetaryImpact) for two locales (us, UK) and a couple of policy inputs to check that negative currencies are displayed properly.

🤖 Generated by Copilot at 9391f67

Summary

🌐💰📊

This pull request improves the currency and locale formatting of the Plotly charts in the PolicyEngine app. It adds new functions currencyString and localeString to the language.js module, and uses them in various chart components. It also adds new locale files locale-en.js and locale-en-us.js to the plotly_locales folder, and supports them in the PolicyEngine.jsx component. Additionally, it fixes some ESLint issues and adds an optional chaining operator to the TextBox.jsx file.

To make the app more appealing
We added some currency and locale dealing
With currencyString and localeString
And some Plotly locale tweaking
We hope to improve the user's feeling

Walkthrough

  • Add currencyString and localeString functions to language.js module to standardize currency and locale formatting (link)
  • Use currencyString function to format text values on chart bars and hover cards in AverageImpactByDecile.jsx and AverageImpactByWealthDecile.jsx (link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Use localeString function to set locale for charts in AverageImpactByDecile.jsx, AverageImpactByWealthDecile.jsx, BudgetaryImpact.jsx, and DetailedBudgetaryImpact.jsx (link, link, link, link)
  • Remove unnecessary commas after maximumFractionDigits parameter in formatVariableValue function calls (link, link, link, link, link, link, link, link, link, link, link, link)
  • Remove unnecessary tickprefix property from y-axis configuration of charts (link, link, link, link)
  • Use aggregateCurrency function to format text values on chart bars and hover cards in BudgetaryImpact.jsx and DetailedBudgetaryImpact.jsx (link, link, link, link)
  • Remove unnecessary commas after other parameters in function calls (link, link, link, link)
  • Add locale-en-us.js and locale-en.js files to plotly_locales folder to define locale settings for US and UK English (link, link)
  • Register loc_en and loc_en_us modules with Plotly library in PolicyEngine.jsx (link, link)
  • Add optional chaining operator to onChange prop of input element in TextBox.jsx (link)

@MaxGhenis MaxGhenis changed the title Fix issue 148 Set ticks on y axis to -£100 instead of £-100 Oct 27, 2023
Copy link
Contributor

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

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

Thanks @abhcs! This looks like a sensible way to do this. I just ran the linter to pass the check, otherwise LGTM.

@nikhilwoodruff nikhilwoodruff merged commit 877fe03 into PolicyEngine:master Oct 27, 2023
2 checks passed
@MaxGhenis
Copy link
Contributor

Thank you @abhcs! This looks so much better. It's been bugging us all for years now haha.
image

@abhcs
Copy link
Collaborator Author

abhcs commented Oct 27, 2023

Glad I could help! (I must have the linter configured incorrectly on my editor, will configure it correctly the next time.)

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.

Set ticks on y axis to -£100 instead of £-100
3 participants