-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use estimated data for trend charts #707
Conversation
Remove a bunch of code that was supporting the use of the unestimated endpoint
Simply munging function based on the presence of population data within the summary response.
531cc89
to
38f172a
Compare
@brendansudol This has been rebased and should be ready for review |
Because we only have estimated data going back to 1995 at the moment
src/components/Explorer.js
Outdated
let { place } = appState.filters | ||
|
||
if (!place && !placeType) { | ||
place = nationalKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did u replace the getPlaceInfo
method in componentDidMount
and render
? i think that helps consolidate this place assignment logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I think that just got removed when I was fixing rebase conflicts. I'll look for it and take a second look
src/util/api.js
Outdated
const getSummary = params => { | ||
const { place } = params | ||
const endpoint = `${API}/counts` | ||
const qs = buildSummaryQueryString(params) | ||
let endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would use ternary here to save a few lines:
const endpoint = (place === nationalKey)
? `${API}/estimates/national`
: `${API}/estimates/states/${lookupUsa(place).toUpperCase()}`
src/components/TrendContainer.js
Outdated
}), | ||
) | ||
)) | ||
const filterDataWithinYears = ({ data, since, until }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if this function should be in '../util/summary'
too and consolidated within the mungeSummaryData
call to avoid multiples passes through the data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this could do the trick:
const mungeSummaryData = (crime, summaries, place, since, until) => {
if (!summaries || !summaries[place]) return false
const keys = Object.keys(summaries)
return summaries[place]
.filter(d => d.year >= since && d.year <= until)
.map(year => {
const data = { date: year.year }
keys.forEach(key => {
const source = key !== place
? summaries[key].find(d => d.year === data.date)
: year
data[key] = {
pop: source.population,
count: source[crime],
rate: source[crime] / source.population * 100000,
}
})
return data
})
}
note the filter
call before the map (and new since
and until
params)
looks good, can review once more and then merge when final tweaks are in |
Move all the data munging and filtering by time period into the summary util
Shoot, didn't mean to merge master in. I've got to get better at the fix merge conflicts inside of github.com workflow, but in any case your feedback was received and implemented @brendansudol |
looks good! |
Uses the
/estimates
endpoint instead of/counts
so that the data displayed includes FBI estimates.Based on #703, which should be merged first.