-
Notifications
You must be signed in to change notification settings - Fork 3
[WIP] Intelligence map #75
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
base: main
Are you sure you want to change the base?
Conversation
d824419 to
ff97faa
Compare
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.
if intelligence-map is just a single file component, then there's no need to nest it within a directory.
look at it from an usage perspective (suppose in storybook):
import IntelligenceMap from '../src/components/intelligence-map'so in this case, the file structure can either be:
./src
|-- components/
|---- intelligence-map.js
or if it grows in complexity where additional modules are needed:
./src
|-- components/
|---- intelligence-map/
|------ index.js <-- main entrypoint
|------ utils.js
|------ hooks.js
|------ some-support-component.js
the import remains the same even if we change the source structure from the first (module) to the second (sub-package) case.
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 see that you used the generic map by copying it in your code. Change this by importing the already existing component in your file and pass only the props you need to it. Look at my GeoCohort map, for example you do not use any legend, no need to pass any legend to it. It is designed to work without that. I believe you use your own onHover event, pass that one, and so on. All the props I passed to the generic map in the GeoCohortMap are needed. So you do the same, clean up the props for the Deck core layer for your map.
stories/intelligence-map.stories.js
Outdated
| return toolTipText | ||
| } | ||
|
|
||
| const useGeoJsonCentroidData = (geoJson) => { |
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.
Similarly, as above, add this to the hooks folder in the index file and import it from there directly into your InteligenceMap comp file.
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 function is only testing purpose for the sample data in storybook which is a parsed based on coordinates version compared to pixel based data in LOCUS atm. A new function will to need to be implemented in LOCUS component to get the Centroid coordinates of each provinces since the data for intelligenceMap in LOCUS currently is parsed and used differently to generate the map.
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 have to look closer, but getting the centroid of each province in a list should be part of the map, it has no place in Locus.... we have the province vector file in react-maps so you can calculate the centroid here as well. This shouldn't be done in Locus. think about it. Only the data values associate with these centroids should be provided from outside react-maps. Will look into it later a bit more.
a43395c to
6f29b9f
Compare
intelligenceMap - lint fix tooltip
…ta values to province
6f29b9f to
b00b66b
Compare
intelligenceMap - fix lint error
b00b66b to
39be763
Compare

Uh oh!
There was an error while loading. Please reload this page.