-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add shapes & points filtering to maps #367
Conversation
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 the filtering could be improved so that we are not in O(N^2). Otherwise LGTM, very cool 👍
app/charts/map/map-state.tsx
Outdated
return data.filter((d) => hierarchyLabels.includes(getLabel(d))); | ||
return data | ||
.filter((d) => dimensionLabels.includes(getLabel(d))) | ||
.filter((d) => hierarchyLabels.includes(getLabel(d))); |
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.
It would be better to have an index to check for inclusion here as otherwise we are looping in N^2.
const index = Object.fromEntries([...dimensionLabel, ...hierarchyLabels].map(l => [l, true]))
return data.filter(d => !!index[getLabel(d)])
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 have modified the code, so it's no longer needed 🥳
When I go there : https://visualize-ad-feat-add-s-8usxwv.herokuapp.com/en/browse/dataset/https%3A%2F%2Fenvironment.ld.admin.ch%2Ffoen%2FCOVID19VaccPersons_sex_w%2F1%2F?previous=%7B%22includeDrafts%22%3Atrue%2C%22order%22%3A%22SCORE%22%2C%22search%22%3A%22covid%22%7D and start a map viz, I have an unexpected error 🤔 Is this because of the dataset ? |
It doesn't work because #364 is not merged yet 😅 |
I had made a quick fix locally to test it on my machine, but I would have to merge #364 to this PR to enable a correct deployment preview 👀 |
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 ! Thanks :)
Closes #342.
This PR adds an ability to filter geoDimension in the right panel. There was a need for some custom logic for shapes, as they are fetched separately from dimension's values.