Skip to content
This repository has been archived by the owner on Jan 28, 2023. It is now read-only.

Issue #41: Budget Spending Per Citizen #137

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

brentlightsey
Copy link

This adds a visualization page to show spending per citizen. The layout is inspired by a New York Times data viz, mentioned in Issue #41 .
budget-per-capita-thumb

The changes include adding the thumbnail to the visualizations page and updating the contributors page.

Link to fork:
https://github.com/brentlightsey/openbudgetokc

…. Adding thumbnail image and updating contributors page.
@brentlightsey
Copy link
Author

Looks like the Travis CI failed because the javascript file I've added isn't node-based. Probably should be. Let me know and I can resubmit if needed.

@smurphy8
Copy link

Looks like it is breaking the tests. Please try and repair.

@brentlightsey
Copy link
Author

Will do.

@brentlightsey
Copy link
Author

OK, it took me multiple tries to get it right (my first PR), but the tests are passing now. There was an assumed javascript dependency that I wasn't checking for.

I was attempting to run npm test locally, but was not successful. Would definitely appreciate any suggestions you have about how to check the commit prior to doing a PR.

@smurphy8
Copy link

@brentlightsey visually I think this looks awesome.
But I did find a glitch when I looked at it in responsive mode.
Looks like the text bleeds together a bit sometimes.
responsive_bug

Also, how are you doing the division, that is what population figure is the data based on?

Sorry for the trouble with npm testing. Can you tell me more about the errors you were having running it?

I usually use yarn test and it works very well.

@brentlightsey
Copy link
Author

Thanks for finding that bug @smurphy8. I had an external dependency on a javascript file that wasn't coming through, so nothing was happening on window resize. That should be fixed with the latest update to the branch. Let me know if it works for you.

I'm using the 2017 metro population of 1,358,452. But I totally agree that we should list that on the page somewhere (I think it's based off of a 2015 estimate done by the US Census Bureau, but it's not as accurate as the true census.) Should I add a note to the top of the page?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants