-
Notifications
You must be signed in to change notification settings - Fork 141
Set benefit increases to negative values in net revenue impact table #2672
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this @v-lerie, just had a few changes for you.
? impact.budget[item.budgetKey] | ||
: item.formula(impact.budget), | ||
item.budgetKey === "benefit_spending_impact" | ||
? -Math.abs(impact.budget[item.budgetKey]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue, blocking: I believe this only half-solves the problem
In cases where the government budget loses revenue (e.g., a new benefit program is introduced), the benefits row should be negative, but in cases where the government budget gains revenue (e.g., benefit cuts), it should be positive. This code will always make it positive.
? impact.budget[item.budgetKey] | ||
: item.formula(impact.budget), | ||
item.budgetKey === "benefit_spending_impact" | ||
? -Math.abs(impact.budget[item.budgetKey]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue, blocking: It's tough to read this nested conditional
Addressing my other comment, can you modify this to be more readable/CLEAN?
} | ||
|
||
if (budgetKey === "benefit_spending_impact") { | ||
impact = -Math.abs(impact); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue, blocking: Here, too budgetary impacts are always negative when they should be negative only sometimes
Description
Fixes #2601
Screenshots