-
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
District numbers and outlines #210
Conversation
lineWidth * 0.35, | ||
14, | ||
lineWidth, | ||
], |
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.
This was just code styling
I've taken another pass to optimize and clean up the way this works a bit. Previously, this functionality added a bunch of Now, whenever a tile loads in, the event listener ( Whenever the assignments updates, the results get sent to the worker which mutates its copy of the data with the updated zones. When zone districts are toggled on or the viewport changes, the worker uses I think there are some additional use cases that will be good to have this amount of geometric data available on the client, and having it on a separate thread means that UX performance impact is minimal. I'm fairly convinced this is a reasonable direction, especially in the interest of keeping the backend lightweight -- eager to hear feedback from you all @raphaellaude @mariogiampieri ! |
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.
This is super slick- it makes a lot of sense to create that separate thread and the user experience was great in my testing. I agree there could be other use cases for a separate worker like- nice work!
Having the dissolved zone areas is nice- I could see a world where we want to outline the zones.
Only minor issue is removing the unused imports but otherwise g2g!
app/src/app/constants/layers.ts
Outdated
@@ -10,6 +10,10 @@ import {getBlocksSource} from './sources'; | |||
import {DocumentObject} from '../utils/api/apiHandlers'; | |||
import {MapStore, useMapStore} from '../store/mapStore'; | |||
import {colorScheme} from './colors'; | |||
import {map, throttle} from 'lodash'; | |||
import {wrap} from 'comlink'; |
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: it appears wrap and map from lodash
are no longer used here, should be removed
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, thank you!
const zone = feature.properties?.zone; | ||
if (!zone) return; | ||
const featureArea = area(feature); | ||
if (!largestDissolvedFeatures[zone] || featureArea > largestDissolvedFeatures[zone].area) { |
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.
O: this makes sense for now given that we are not enforcing contiguity on zones, but could likely be refactored later when that rule is enforced
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.
Agreed! I added this note in a TODO
along with a couple other future directions we could add
Thank you @mariogiampieri! I just updated the unused imports and added a couple notes to our future selves. |
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, nice work!
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! great work
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!
PS This closes a few issues: |
Adds zone numbers to map. Hacky approach, but mostly works
Description
Reviewers