search

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

Separate enhanced CPS from region selector #1150

Closed anth-volk closed 4 months ago

anth-volk commented 4 months ago

Description

Fixes #1088; fixes #1153. At present, the back end is only able to utilize enhanced CPS data with the US country package. It currently categorizes the US with this enhanced data as a separate region, not as a subset of the US region, creating complications for the UX. This PR creates a temporary component, meant to last until the back end is improved, that visually displays enhanced CPS data as part of a new "data" selector on the right-hand panel that only displays if the US is selected as the target "region."

Changes

This creates a new component, DatasetSelector, that conditionally renders if the user selects "US" as their "region." this also removes US (enhanced CPS) from the RegionSelector component. It does all of this manually and imperatively, meaning that improving the back end and removing the enhanced CPS as a region should still be prioritized, as these components are now inherently fragile. The component also manages state setting for the region URL param, but so does RegionSelector, creating potential side effects. One such effect is that clicking "US" in RegionSelector reinstates the default data; this component handles that by defining an unnecessary key prop that ensures re-render upon this change. Other side effects may exist due to the fact that two inputs are simultaneously defining the region URL param.

Screenshots

Loom video available here.

Tests

This PR does not include any tests. I'd be happy to push this to production without them, as it's relatively small and ideally temporary, or would be happy to await the merging of #1123, which repairs Jest such that tests can actually be written without import problems.

MaxGhenis commented 4 months ago

How about we instead display it as a tabbed radio button between the drop downs and teal buttons? And show it for all geographies, just disabled for all but US.

Two radios can be "Base CPS" and "Enhanced CPS (experimental)" with a hover over enhanced when disabled "We currently only offer the Enhanced CPS for nationwide analysis."

MaxGhenis commented 4 months ago

We could also disable the enhanced option for 2021 (see https://github.com/PolicyEngine/policyengine-app/issues/1153)

anth-volk commented 4 months ago

By "tabbed" above, are you referring to something like the below @MaxGhenis?

Screen Shot 2024-01-11 at 4 48 28 PM
MaxGhenis commented 4 months ago

yep

anth-volk commented 4 months ago

Do you want this to display for the UK, as well, or to only be present on the US site?

anth-volk commented 4 months ago

Also, at the moment, this is the best I'm getting in terms of spacing with the two options because "Enhanced CPS (experimental)" is such a large piece of text. I could stack them, but would probably have to rewrite this not to use Ant Design, or could somehow change the text to fit better. What are your thoughts?

Screen Shot 2024-01-11 at 6 41 14 PM
MaxGhenis commented 4 months ago

What if we move experimental below the button?

More broadly I wonder if using a bit more space to describe the options would provide more clarity. Like "Geography" above that drop down, "Year", "Baseline policy" and then "Data" could be "Basic" and "Enhanced" with "Current Population Survey" on hover

anth-volk commented 4 months ago

I can work up something that would change this a bit more so as to make it more aesthetically pleasing. That said, I'd advocate for either adding this as another drop-down or as essentially two buttons, but stacked, as I think this horizontal structure is going to always crowd text out. If you look at the width of this panel prior to the mobile breakpoint (at which this becomes a hidden panel accessible from the bottom), it gets very narrow.

MaxGhenis commented 4 months ago

Maybe we don't need the basic. Just a switch to flip on enhanced data. I think I've seen other examples of "experimental" in small font below stuff

anth-volk commented 4 months ago

Before continuing, here are three potential mockups for this component. Feel free to let me know what you think, and if one is preferred over the others (even with edits), I can proceed on that one.

  1. Long form, tabbed radio

    Screen Shot 2024-01-11 at 8 30 29 PM

  2. Long form, slider

    Screen Shot 2024-01-11 at 8 46 08 PM

  3. Short form, slider

    Screen Shot 2024-01-11 at 8 51 34 PM

Both would support disabling the text and slider/radio, and both would enable a tooltip on hover with the explanation you provided earlier.

MaxGhenis commented 4 months ago

Let's go with 2 while it's experimental (more subtle) and then we can consider changing to the tabbed view after full launch

anth-volk commented 4 months ago

Hopefully I'll have this done today; if not, I have a few hours to make up over the weekend, so ideally will be ready for review before Monday.

anth-volk commented 4 months ago

This is nearly done, just struggling with the beast that is Jest. Should have this, with tests, finished this weekend.

anth-volk commented 4 months ago

This is now complete, including stylistic changes and tests. A Loom video demonstrating the component in action is available at https://www.loom.com/share/fadc33d55a1940059ce01ab5984effad?sid=e9baec3e-12eb-41b2-a092-ceab5bb91166. Now also fixes #1153.

MaxGhenis commented 4 months ago

Thanks, could you show how it looks on mobile? Also let's use use rather than utilize please

anth-volk commented 4 months ago

The mobile view is available here; this also ensures that the tooltip we show fires on touch on mobile, instead of on hover. The change from Utilize to Use has also been implemented.