search

Hyperobjekt / north-texas-evictions

1 stars 1 forks source link

Feat: Time Comparison #139

Closed Lane closed 2 years ago

Lane commented 2 years ago

Changes

netlify[bot] commented 2 years ago

✔️ Deploy Preview for north-texas-evictions ready!

🔨 Explore the source changes: 411d1fb36002dd1888f8b75c1edb02d69bb6294f

🔍 Inspect the deploy log: https://app.netlify.com/sites/north-texas-evictions/deploys/61da2b0e2102860007a14bf7

😎 Browse the preview: https://deploy-preview-139--north-texas-evictions.netlify.app/

alegzandach commented 2 years ago

@Lane Now that it's done of course, I was thinking it might make more sense to rearchitect some pieces into their own components. Basically, a general use chart component that the timeComparison and timeSeries (maybe renamed to locationComparison?) components can use, and abstract out some of the stuff in the current timeSeries component, like the renderTooltip function (I ended up making a custom one for the comparison chart and passing it in). It would probably take a few hours though...just a thought!

Lane commented 2 years ago

@alegzandach thanks again for your work on this, it looks great! 🙌

let's hold off on refactoring for now, i think the being able to pass in a custom tooltip function does the trick. a couple things to fix up:

code duplication

i'm seeing a new utils file in the time comparison module that is duplicating many of the functions already provided in the time series utils. could you update to import the existing utils instead of duplicating?

handle cases where 2019 data is unavailable

2019 data is not available for Tarrant County:

image

we should add a few things for these locations:

image

size / margin adjustment

let's bring the chart height down too ~300px (from 400px). also, it looks like it current has a top and right margin, let's drop those.

  1. update the base /src/TimeSeries/components/TimeSeriesChart.js component so that any additional props are passed on to the XYChart component.
  2. update the <TimeComparisonChart /> with margin and height props. these seem to work well:
<TimeSeriesChart
    // ... other props
    margin={{ right: 0, top: 16, left: 32, bottom: 32 }}
    height={300}
/>

image