-
-
Notifications
You must be signed in to change notification settings - Fork 157
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 and use MapLibre in React if running Terrastories in offline mode #943
Conversation
Follow up PR to take care of the Rails part: #944 |
As of today, this PR is pulling in both mapbox and maplibre which will unnecessarily bloat our assets. We will need to find a way to dynamically determine pulling only one of the two libraries as per the Rails environment, env vars, or a different means. One possibility might be to incorporate these mapbox/maplibre React components: https://visgl.github.io/react-map-gl/docs |
I implemented dynamic importing to ensure only one library is being pulled on the React side. I tried to include the CSS imports as well, but that resulted in things like the map controls or minimap (for Mapbox) not rendering at all. In these screenshots you can see that the non-utilized map library were not pulled by Webpack: (Also, small but meaningful thing: I had the Minimap use the same style as the map itself.) |
Would be great to get this one in the new |
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 looks good overall. I have a couple of questions/clarifications on the import placement and usage of the map library switching.
if (this.props.useLocalMapServer) { | ||
import('!maplibre-gl').then(module => { | ||
this.initializeMap(module.default, true); | ||
}); | ||
} else { | ||
Promise.all([ | ||
import('!mapbox-gl'), | ||
import('../vendor/mapboxgl-control-minimap.js') | ||
]).then(([mapboxGLModule, minimapModule]) => { | ||
this.Minimap = minimapModule.default; | ||
this.initializeMap(mapboxGLModule.default, false); | ||
}); | ||
} | ||
} |
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'm wondering if we could move the conditional imports into the constructor rather than on mount. We likely want the libraries imported and ready to go before the component has mounted and our rendering starts. I have an unsubstantiated concern that we may inadvertently re-import these libraries if React decides it needs to remount.
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.
So, I couldn't quite figure this out -- to the best of my knowledge and research, dynamic imports are inherently asynchronous, whereas constructors are not and can't handle Promises. Do you have any suggestions for how to handle this via the constructor?
However, I did implement a check to see if the modules have already been imported by setting that in the state. If they haven't, we import and store the modules, and if not, we skip and call initializeMap
with the already imported modules.
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.
👍
Addresses one chunk of the work described in #882.
This PR implements MapLibre for the React front end map at the /home/ route, if in offline mode.
maplibre-gl
to package.json.Map.jsx
, I created a new propmapGL
which is set to Maplibre ifuseLocalMapServer
is true (with fallback Mapbox), and applied it across the code.mapboxStyle
to be namedmapStyle
instead.The next chunk of work will be on applying MapLibre in Rails if in offline mode, but that be a separate (follow-on) PR.