Skip to content
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

Refactor map component #69

Merged
merged 14 commits into from
Nov 10, 2021
Merged

Refactor map component #69

merged 14 commits into from
Nov 10, 2021

Conversation

laudickson
Copy link
Collaborator

First few thoughts trying out a refactored more 'general' component for map

Changed from vanilla js mapbox library to react-map-gl pacakge. I've found it is WAY more react-typescript friendly and felt very little friction with the type-system

This is pretty rough and tbh the more I played with ReactMapGL I questioned if even it's necessary to have a generic in-app Map component and just use ReactMapGL component directly when you need to - their API and documentation is pretty comprehensive and they give lots of examples that offers a lot of great patterns

https://visgl.github.io/react-map-gl/examples/geojson
https://visgl.github.io/react-map-gl/docs

Also used some things from Brad's PR https://github.com/codeforboston/mattapan-mapping/blob/b909bec69422fbd94183af1a95c88824305d8511/src/components/Map/Map.tsx v useful

This PR is in draft for now, I'll spend a couple more shower thoughts and see if I can polish this further. The 'Explore' page is playground for now to see how an instance of Map component could play out (still using not-curated-data)

@netlify
Copy link

netlify bot commented Nov 5, 2021

✔️ Deploy Preview for mattapan-mapping ready!

🔨 Explore the source changes: a7f1356

🔍 Inspect the deploy log: https://app.netlify.com/sites/mattapan-mapping/deploys/618c1ec7c6e11a000944ea89

😎 Browse the preview: https://deploy-preview-69--mattapan-mapping.netlify.app/

@laudickson
Copy link
Collaborator Author

Also, I deleted package.json in this changeset incase things conflate with yarn packages (I think I had it from when originally I set up CRA)

satellite: 'mapbox://styles/mapbox/satellite-v9',
satelliteStreet: 'mapbox://styles/mapbox/satellite-streets-v11',
navigationDay: 'mapbox://styles/mapbox/navigation-day-v1',
navigationNight: 'mapbox://styles/mapbox/navigation-night-v1',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Threw in map styles into the Theme and worked pretty well with intellisense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

@@ -7,6 +7,8 @@ interface Theme {
fonts: Fonts;

colors: Colors;

map: MapStyles;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay or nay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay

@laudickson laudickson force-pushed the refactor-map-component branch from ace7b09 to d06bff9 Compare November 7, 2021 20:12
@laudickson laudickson closed this Nov 8, 2021
@thadk
Copy link
Member

thadk commented Nov 8, 2021

Thanks for choosing our opinion(s) on the Mapbox library setup, @laudickson!

Not sure why the deploy preview situation is so overloaded but this is the one that built I think, though the mapbox token might not be right still https://deploy-preview-69--mattapan-mapping.netlify.app/#/
Do you happen to have 1-3 netlify's going in your account there?

Final build was most successful I guess: https://mattapan-mapping.netlify.app/explore

@laudickson laudickson reopened this Nov 8, 2021
@laudickson
Copy link
Collaborator Author

laudickson commented Nov 8, 2021

I'll go over and share more in-depth with group tomorrow, but I'm somewhat settling on this pattern for now -

  • All Map-like components in MapAtoms; I don't think having Map in organism makes sense anymore
  • Whenever you want to make a map with a geojson data-type (or multiple geojsons), the geojson you want to import has to have a corresponding 'layer' with it; basically a json that describes how the geojson will be styled (follows LayerProp as described by react-map-gl`

Ex. pattern loosely looks like

import geoJsonData1 from './geoJsonData1';
import geoJsonData2 from './geoJsonData2';

const layer1 = { id: 'foo', type: 'line', paint: { 'line-color': 'orange' } };
const layer2 = { id: 'bar', type: 'fill', paint: {fill-color': 'purple' } };

      <Map mapStyle={ Theme.map.dark }>
        <>
          <MapSource data={ geoJsonData1 } layer={ layer1 } />
          <MapSource data={ geoJsonData2 } layer={ layer2} />
        </>
      </Map>

Concerns/issues:

  • Type casting geojson data before able to actually put into as prop value (omitted in example)
  • necessary fragment brackets because React children only expects 1 child (how to get around?)
  • questioning the whole pattern itself / appropriate in MapAtoms?
  • CRA doesn't allow imports outside of src directory; meaning assets will have to be moved into that (and potentially renamed and change to .ts extension?)
  • Need to create new issue that creates control panel component for toggling a map source

id: 'greaterMattapanZoning',
type: 'line',
paint: {
'line-color': 'yellow',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a color as defined from color palette

@laudickson laudickson marked this pull request as ready for review November 8, 2021 23:15
@laudickson
Copy link
Collaborator Author

laudickson commented Nov 8, 2021

Not sure why the deploy preview situation is so overloaded but this is the one that built I think, though the mapbox token might not be right still https://deploy-preview-69--mattapan-mapping.netlify.app/#/ Do you happen to have 1-3 netlify's going in your account there?

Final build was most successful I guess: https://mattapan-mapping.netlify.app/explore

@thadk good look there, it has been working locally for me pretty much all this time but turns out it was related to this visgl/react-map-gl#1266 and this version of mapbox isn't transpiring correctly. Lots of suggested solutions in that thread, but this one seemed to work as far as from looking at the deploy preview (craco ftw once again!)

@laudickson
Copy link
Collaborator Author

and move assets and boundaries into public folder and use from there

@laudickson laudickson merged commit c5ebdc4 into main Nov 10, 2021
@laudickson laudickson deleted the refactor-map-component branch November 10, 2021 19:42
@aniyip aniyip added this to the Explore Page milestone Feb 1, 2022
@aniyip aniyip added the enhancement New feature or request label Feb 1, 2022
@aniyip aniyip modified the milestone: Map Page Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants