search

PolicyEngine / policyengine-app

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

Reskin fonts and layout changes #1670

Closed nikhilwoodruff closed 2 weeks ago

nikhilwoodruff commented 2 weeks ago

Description

Improves visual consistency in the calculator, changes the ordering of the main calculator panes, and other assorted visual fixes.

Changes

There's quite a lot and they're all pretty distributed. But very little structural changes, mostly changing values and adding text.

Screenshots

https://github.com/PolicyEngine/policyengine-app/assets/35577657/dac51d5b-766a-4c42-bc6b-8349acf2bbc5

Tests

None added.

anth-volk commented 2 weeks ago

@nikhilwoodruff Is this meant to include the DistrictImpact component?

nikhilwoodruff commented 2 weeks ago

Looks like this slows down the US app significantly- not sure yet why.

nikhilwoodruff commented 2 weeks ago

@nikhilwoodruff Is this meant to include the DistrictImpact component?

Ah thanks good catch.

anth-volk commented 2 weeks ago

Very true on the slowing down of the app - it took me a good 5 seconds to select an existing policy while the app just kind of sat.

Edit: To be clear for others reading this, the edits don't slow down the computation time, only the time it takes to actually create and edit a policy

anth-volk commented 2 weeks ago

Additionally, could we make the underlined header items something like black? I get that you're shooting for them being understated, but I'm wondering if there's something else we could do to express that. The contrast is very low, and I think for accessibility purposes, it'd be best to raise it.

Screen Shot 2024-04-30 at 4 37 02 PM
anth-volk commented 2 weeks ago

I think that these subtitles, too, feel a bit off because of their coloring or something, though, if I'm being honest, I'm not sure what the right approach is to them

Screen Shot 2024-04-30 at 4 42 13 PM
anth-volk commented 2 weeks ago

And you might want to un-bold this

Screen Shot 2024-04-30 at 4 42 31 PM
anth-volk commented 2 weeks ago

The expand button below currently does nothing.

Screen Shot 2024-04-30 at 4 45 29 PM
anth-volk commented 2 weeks ago

When I run the household calculator, I get a runtime error of "parameter is undefined" and the app crashes, unfortunately. That said, the household calculator is as fast as before, so I wonder if there isn't some code in the policy calculator side that's creating a much higher number of re-renders?

Screen Shot 2024-04-30 at 5 00 12 PM
anth-volk commented 2 weeks ago

Additionally, the VariableEditor component has more padding at its top than the age and other inputs, creating a weird visual shift as you cycle through the app. Also, with the left alignment of the component, I think it creates a strange visual when adding multiple people, as visible below.

Screen Shot 2024-04-30 at 4 52 46 PM

What if the variable/parameter inputs were centered? Alternatively, the inputs could be structured like a standard form with label on top, block input below, but that seems like overkill for some of these, or you could grid the data and have a max-content width on the left-hand side, creating a large gap that ensures the inputs are always aligned together, but I know you had said previously that you weren't in love with that either.

anth-volk commented 2 weeks ago

Also, for some reason, this modal has rounded corners. Incidentally, this code doesn't use Ant Design's ConfigProvider, does it?

Screen Shot 2024-04-30 at 4 53 02 PM
nikhilwoodruff commented 2 weeks ago

@anth-volk on the dark-grey: I'm going to go ahead as I think we should still do a full accessibility review, and I do think we have quite a big problem with distracting, unclear focus points on the current app which impedes accessibility itself. Filed https://github.com/PolicyEngine/policyengine-app/issues/1671 to follow up on.

nikhilwoodruff commented 2 weeks ago

Additionally, the VariableEditor component has more padding at its top than the age and other inputs, creating a weird visual shift as you cycle through the app. Also, with the left alignment of the component, I think it creates a strange visual when adding multiple people, as visible below.

Screen Shot 2024-04-30 at 4 52 46 PM

What if the variable/parameter inputs were centered? Alternatively, the inputs could be structured like a standard form with label on top, block input below, but that seems like overkill for some of these, or you could grid the data and have a max-content width on the left-hand side, creating a large gap that ensures the inputs are always aligned together, but I know you had said previously that you weren't in love with that either.

@anth-volk - since we're moving to left-aligned I think we should be consistent wherever we can. Fixed here with a minimum width style.

image
nikhilwoodruff commented 2 weeks ago

@anth-volk: I've downgraded a test to skip mode in one module: Dialogs.test.js. I think it's broken- I've tested in multiple browsers and attached a video below. Otherwise I think we're OK.

https://github.com/PolicyEngine/policyengine-app/assets/35577657/6ee56a67-64e7-4381-98ea-7ab6a0114a4e

nikhilwoodruff commented 2 weeks ago

@anth-volk FYI the linter doesn't accept disabled tests being anywhere so I'm removing it- could you take a look in the future at it? It's still in the repo history so not gone.

nikhilwoodruff commented 2 weeks ago

Alright, merging. Thanks @MaxGhenis and @anth-volk for the detailed feedback along the way!