Public views #452
Public views #452
Conversation
nofurtherinformation
temporarily deployed
to
pr-452
GitHub Actions
Inactive
nofurtherinformation
temporarily deployed
to
pr-452
GitHub Actions
Inactive
nofurtherinformation
had a problem deploying
to
pr-452
GitHub Actions
Failure
nofurtherinformation
temporarily deployed
to
pr-452
GitHub Actions
Inactive
nofurtherinformation
had a problem deploying
to
pr-452
GitHub Actions
Failure
nofurtherinformation
temporarily deployed
to
pr-452
GitHub Actions
Inactive
nofurtherinformation
temporarily deployed
to
pr-452
GitHub Actions
Inactive
nofurtherinformation
temporarily deployed
to
pr-452
GitHub Actions
Inactive
| const layerFilter = useLayerFilter(child); | ||
|
|
||
| const layerProps = useMemo(() => { | ||
| const overlayLayerProps: LayerProps = { |
There was a problem hiding this comment.
The variable name overlayLayerProps is slightly confusing because it defines configuration for layers regardless of whether they are overlays or not, as we see with the use of isOverlay here.
What layer this actually represent? Is it more accurate to say polyLayerProps?
| variant: demographicVariant, | ||
| mapRef: _map, | ||
| mapDocument, | ||
| numberOfBins: numberOfBins || 5, |
There was a problem hiding this comment.
Should the default number of bins be stored as a constant and then imported here?
| if (!ParquetWorker) { | ||
| throw new Error('ParquetWorker not found'); | ||
| } | ||
| if (mapDocument.document_id === 'anonymous') { |
There was a problem hiding this comment.
I see the anonymous document id referenced several times across the new and updated files. Perhaps it is worth making this a constant value as well?
| const response = await get<Array<PublicDistrictData>>(`document/${mapDocument?.public_id}/stats`)( | ||
| {} | ||
| ); | ||
| if (response.ok) { |
There was a problem hiding this comment.
I noticed that HTTP request errors are swallowed here. Would it be worthwhile to bubble up the error instead so TanStack Query can attempt a retry in useMapBrowserEvents?
| if (hash === this.hash) return; | ||
| this.availableColumns = columns; | ||
| this.table = table(data).derive(getColumnDerives(columns)).dedupe('path'); | ||
| update(data: {columns: AllTabularColumns[number][]; results: ColumnarTableData}): void { |
There was a problem hiding this comment.
The update method doc string is now out-of-date. More generally, if time permits, it would be nice to see more doc strings throughout the code base.
| NOW() AS updated_at | ||
| FROM geos | ||
| {f"INNER JOIN gerrydb.{gerrydb_table} demo ON demo.path = geos.geo_id" if (gerrydb_table and demographic_json) else ""} | ||
| WHERE zone IS NOT NULL |
There was a problem hiding this comment.
What does it mean to get rid of this clause? Was it redundant?
|
|
||
| return ( | ||
| <Source id={BLOCK_SOURCE_ID} type="geojson" data={getPublicMapData()} promoteId="zone"> | ||
| {!isDemographicMap && ( |
There was a problem hiding this comment.
Perhaps a ternary expression would be cleaner here?
LaunaG
left a comment
LaunaG
left a comment
There was a problem hiding this comment.
Overall, this PR looks good to me, and the new functionality seems to work in the browser!
I only have a few minor questions and suggestions related to code clean-up and error handling.
Currently, public views load in all the relevant data in the same way that the map editor does. This is generally fine, but leads to heavier database load for loading assignments (heavy for a complex plan) and client load for map data and demographic data.
This PR seeks to simplify this by using the recently implemented
GET api/document/{document_id}/statsendpoint which generates a simple geojson with geoprocessing unioned districts and relevant stats.Description
Reviewers
Checklist
Screenshots (if applicable):