GreenInfo-Network / nyc-crash-mapper-chart-view

Chart view for NYC Crash Mapper that allows for viewing Trends, Comparing, and Ranking of various NYC geographies
http://vis.crashmapper.org
MIT License
2 stars 1 forks source link

Create vehicle page #119

Closed taylorg198739 closed 4 years ago

danrademacher commented 4 years ago

Overall this is looking good! In future, it would be ideal if you could commit smaller changes and many more commits for a PR like this, so we can see how the changes build up, rather than a single 1400-line commit. But the result is working well and looking good, so that's great.

A few items to resolve here:

Fixes and minor changes

We consume that area from URL params here https://github.com/GreenInfo-Network/nyc-crash-mapper-chart-view/blob/master/src/common/sqlQueries.js#L220-L227, and it works int eh other views. I am not sure yet why it is not working with your new Vehicle detail query, since I think it should just append as another WHERE clause. Can you take a look? Maybe there's a GROUP BY dependency we need to resolve.

The site overall is not super responsive, but a bit less space between the bars and first pie would probably solve this, and ideally allow to flex so it can be wider on wide screens.

Keep the text colors, but let's make the font sizes and parens around number match to the PERIOD labels, which are more legible.

Bigger changes

these are a bit bigger.

If citywide load time is too long, we could also consider adding a message like "Select a geography to view vehicle information." But a default citywide would be much better!

so in this case the bar max would be set by 969 on the left: image