-
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
Maps integration #240
Maps integration #240
Conversation
Apart from the small stuff I commented, the code is clear, thanks for having the smallish commits ;) |
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! Added a few comments but overall seems to work nicely 👏
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 do not see any blocker for here. LGTM :) Great job !
A pattern I have noticed though in the code was the use of find
and filter
where indexing could be used and be clearer / faster.
(o) => | ||
o[chartConfig.fields.areaLayer.componentIri] === d.properties!.label | ||
); | ||
}); |
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.
You could use an index label -> observations here so that this step is O(nlogn) instead of O(n2).
observation: data?.dataCubeByIri?.observations.data.find( | ||
(o) => | ||
o[chartConfig.fields.areaLayer.componentIri] === d.properties!.label | ||
), |
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.
The observtions by label index could be used here.
label: d.label, | ||
observation: observations?.find( | ||
(o) => o[symbolGeoDimensionIri] === d.label | ||
), |
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.
A observations by label index could be useful. Since it is needed in a lot of places, I wonder if we should have a DataProvider somewhere in the tree that does the indexing so that children do not have to do this repeatedly, what do you think ? (this is not the scope of the PR).
allIris: dimensionIris, | ||
narrowerValues, | ||
}); | ||
}; |
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.
Nit: this could be moved into the dimension-hierarchy file, and a test could be good :)
return data.filter((d) => hierarchyLabels.includes(getLabel(d))); | ||
} | ||
|
||
return 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.
Instead of having a callback, I think we could directly index the shapes by level so that we can use them directly afterwards without recomputation.
const dataByLevel = useMemo(() => { groupBy(data, x=> getLevelByLabel(x.label)) }
ba1b5b6
to
a9c074a
Compare
a9c074a
to
56ab798
Compare
…mall improvements
Draft of integrating maps to the app. You can test the feature by using a "Covid 19 Vaccinated Persons" dataset (and pass e.g. 2021-11-20 to the date filter to actually see some data :) ).
Currently all the shapes are fetched dynamically (so the static files are not used).
TODO:
TBD:
Nice to have: