-
Notifications
You must be signed in to change notification settings - Fork 1
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
Chart enhancements + stub of places
and districtingProblems
#167
Conversation
Ok, the chart is working as intended now, with a pin put in the |
places
and districtingProblems
port overplaces
and districtingProblems
places
and districtingProblems
places
and districtingProblems
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 added a couple small comments but FE stuff looks good to me for user testing. I didn't look too deeply into the backend work here for now
} | ||
}, [summaryStats, numDistricts]); | ||
|
||
const calculateChartObject = () => { |
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.
PP: calculateChartObject
and the useEffect
on 72 would be more readable with useMemo
instead
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.
good catch
useEffect(() => { | ||
if (summaryStats && numDistricts) { | ||
const totalPop = summaryStats.totpop ? getEntryTotal(summaryStats.totpop.data) : 0; | ||
setIdealPopulation(totalPop / numDistricts); |
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.
PP: This could make more sense as mapStore.summaryStats.idealPopulation
-- are there other places we'd want to have this number available?
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.
Could be queries.ts:72
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.
nice! good call
… feat/chart-enhancements
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.
Only one blocking comment to delete unused UDF. Let's handle the two_or_more_races_pop
issue in a separate PR to get this in.
@@ -44,30 +88,49 @@ export const HorizontalBar = () => { | |||
return ( | |||
<Flex gap="3" direction="column"> | |||
<Heading as="h3" size="3"> | |||
Population by Zone | |||
Population by District |
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.
PP: looks like Moon has been using sentence case which I think is better style -> Population by district
|
||
export const P1ZoneSummaryStatsKeys = [ | ||
'other_pop', | ||
'asian_pop', | ||
'amin_pop', | ||
'nhpi_pop', | ||
'black_pop', | ||
'white_pop' | ||
] as const | ||
'white_pop', |
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: Will want to add two_or_more_races_pop
here
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.
Will handle in separate PR
@@ -192,7 +192,7 @@ export const CleanedP1ZoneSummaryStatsKeys = [ | |||
'nhpi_pop_pct', | |||
'black_pop_pct', | |||
'white_pop_pct', |
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: and add two_or_more_races_pop
here
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.
Will handle in separate PR
backend/app/main.py
Outdated
@@ -5,7 +5,7 @@ | |||
from starlette.middleware.cors import CORSMiddleware | |||
from sqlalchemy.dialects.postgresql import insert | |||
import logging | |||
from sqlalchemy import bindparam | |||
from sqlalchemy import bindparam, func |
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.
Q: is func used here? I don't see it in the diff
@@ -0,0 +1,40 @@ | |||
DROP FUNCTION IF EXISTS get_summary_stats_pop_totals; |
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.
H: This function isn't being used now, no? We can delete in favor of using the either get_summary_p1_totals
or get_summary_p4_totals
(and eventually others...)?
amin_pop BIGINT, | ||
nhpi_pop BIGINT, | ||
black_pop BIGINT, | ||
white_pop BIGINT |
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.
H: Even though we should remove this function, we'll need to add two_or_more_races_pop
to get_summary_p1_totals
.
pipelines/simple_elt/main.py
Outdated
if __name__ == "__main__": | ||
cli() | ||
|
||
|
||
def upsert_places_and_problems(): |
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.
PP: Move above if __name__ == "__main__"
but add raise NotImplementedError
in the function body. Or just warnings.warn("Not implemented")
or something
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.
lgtm
Description
MapDocument
properties, including a target number of districts (manually set for now) and a total population. This chart behavior includes:Re: places and problems, to circle back on:
DistrictrPlace
andDistrictrProblem
models, following v1 places / problems and syntax that a user selects to start a session. This was in service of getting the proper target # districts for each map. I suspect that we'll want to discuss and revise how these objects fit in with our data model. For now at least, they provide a target for each test map, based on districtr v1 statewide places and problemsReviewers
Checklist
Screenshots (if applicable):