PolicyEngine / policyengine-app

PolicyEngine's free web app for computing the impact of public policy.
GNU Affero General Public License v3.0
35 stars 89 forks source link

Split up decile impact by total/income/substitution #1790

Closed MaxGhenis closed 1 week ago

MaxGhenis commented 2 weeks ago

This chart is too complex IMO. Let's split them up or use radio buttons. Also income and substitution effects are the same color

image
anth-volk commented 1 week ago

Working on this currently, but the our implementation of DisplayImpactChart is so tightly coupled to the logic that the radio buttons, which are a far better implementation in my opinion, are proving difficult, as it's not possible to use stateful variables within the individual impact chart components due to how they're fed into DisplayImpactChart. I'll see about writing some sort of wrapper component to allow this to occur.

anth-volk commented 1 week ago

I've looked into this some more and have found no way of actually getting the radio button logic to work here. Longer-term, I think the interface by which impact charts are displayed is coupled far too closely with one particular logic, and it should be rewritten in a way that simplifies output and allows each output pane to act as its own page.

That said, in the short term, I could break these out into three separate tabs under "overall" (or two, if you'd like the total effect line on the income and substitution effect charts, instead of giving it its own tab). Would you like me to proceed as such?

MaxGhenis commented 1 week ago

Out of curiosity, are the household charts with radio buttons and drop-downs written more flexibly?

MaxGhenis commented 1 week ago

Yes, separate tabs are fine

anth-volk commented 1 week ago

Much more flexibly. They're basically their own components, whereas the policy ones are fed as arguments into a function invoked by a higher-order component.

The policy-side structure looks something like this:

getImpactReps, meanwhile, takes a subdomain (like policyOutput.budgetaryImpact.overall) and a series of props and uses the subdomain as a key within a data structure that associates each subdomain with its relevant ImpactChart. It returns the JSX of the relevant chart component with props passed in.

This makes it impossible to use any hook within the charts, particularly useState, as React expects a static number of hooks equivalent to how ever many existed on the first chart output page the user loaded, but then when a page with its own hooks is loaded, that number changes, causing React to crash.

anth-volk commented 1 week ago

I've been working on this, initially trying to implement an alternative interface, but when it became clear it'd be too involved, I just created separate components for each of the individual items.

I'll send a video once it's done, but I'm struck by how awkward it is. What if, until we can rewrite the interface to enable more flexibility within the output panel components, we just do the following:

  1. Recolor one of the two effects so that it's not gray
  2. Stack the bars (since they're cumulative)?

The stacked bars could even display both a total and a breakout on the hoverbox

anth-volk commented 1 week ago

Here's how the app looks with the tabbed pages.

https://github.com/PolicyEngine/policyengine-app/assets/14987227/302a2265-ac44-4c07-ab50-7446fa985737

MaxGhenis commented 1 week ago

Let's make them all columns

anth-volk commented 1 week ago

PR opened (#1821) to do so. Looking forward to hearing any critiques, desired changes, etc.