-
Notifications
You must be signed in to change notification settings - Fork 0
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 Support for Vector Tile Basemaps #26
base: master
Are you sure you want to change the base?
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.
Really excellent work, and with such a good result. I've suggested fairly minor changes.
import vectorTileStyling from '../../styles/3005.js'; | ||
import L from 'leaflet'; | ||
|
||
export default class BCVectorBaseMap extends PureComponent { |
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.
Class-based components are deprecated in React. I encourage you not to create new ones using them. Writing (and rewriting) components as functional components is straightforward and brings a lot of benefits.
import 'proj4leaflet'; | ||
import { projCRSOptions } from '../../utils/crs'; | ||
|
||
const LabelsLayer = ({ wmsUrl, wmsOptions }) => { |
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.
Why do we need to create our own component for this when Leaflet contains TileLayer.WMS?
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.
My intention when creating a separate component for the label layer was to render the labels independently of the feature layer. This way, if one layer is rendered faster than the other it would give users a higher perceived performance by presenting some information in a tile space. This got me thinking of the layer independence and I've added a zIndex option so that labels will render on top of the climate rasters, making them even more readable.
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 get the reason for having a separate label layer and feature layer. That's excellent.
But the label layer is just a WMS layer, and that is already built in to Leaflet. I think you can just use that, passing in the appropriate parameters to make it fetch the label tiles. What am I missing here?
(Possibly you are not distinguishing between a component and what it renders, which is parallel to the distinction between a class and an instance. I'm saying we don't need a new component, but we do need of course a separate instance of it for the label layer.)
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 clean breakdown into components here.
This module file is pretty long ... which is generally to be avoided when possible. Consider splitting out the components that are internal here into separate modules and importing them where needed.
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.
Agreed, I've split GenericVectorBaseMaps apart so that the labels and vectorgrid layer are separate modules.
a44c25e
to
255a6be
Compare
255a6be
to
0dc6986
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.
Great work, Quintin! And the code is nice and tight now, good separation of abstraction from usage.
<WMSTileLayer | ||
url={wmsUrl} | ||
{...wmsLayerOptions} | ||
/> |
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.
👍
mapRef, | ||
vectorTileOptions, | ||
wmsUrl, | ||
wmsLayerOptions, |
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 wonder if the wms...
props should be given names that indicate their use, something more specific like wmsLabelLayerUrl
, etc.? Or do you think that the GenericVectorBaseMap
is/could be a more generalized tool, where the WMS layer would be used for other things?
Pull Request: Add Support for Vector Tile Basemaps
Summary:
This PR introduces support for vector tile base maps in our map applications to support dynamic, client-side styling. There may be some improvements to feature styling and labels based on feedback from CSG and RCI, but there are no significant changes planned for the components in this PR.
Key Changes:
GenericVectorBaseMap.js
:GenericVectorBaseMap
, to handle the rendering of a vector basemap and a wms label layer.VectorGridLayer
that is a wrapper for Leaflet.VectorGrid.WMSTileLayer
wrapper fromreact-leaflet
BCVectorBaseMap.js
:EPSG:3005
projection.GenericVectorBaseMap
with region-specific configurations, such as a custom BC Albers projection definition and extent-tuned feature rendering based on zoom level.vectorTileStyling.js
:legendColored.js
,legendAchromatic
andlegendSelector.js
:legendColored
,legendAchromatic
).legendSelector
mechanism based on an environment variable (REACT_APP_LEGEND_TYPE
), allowing for easy switching between different styles.Associated Environment variables:
REACT_APP_BC_VECTOR_BASE_MAP_TILES_URL
mapbox-vector-tile
format.REACT_APP_LABELS_WMS_URL
Example:
Purpose:
These changes add vector tile base map support, enabling:
Testing Performed:
Demos:
Greyscale with 0.6 opacity raster overlay:
https://beehive.pacificclimate.org/plan2adapt/app
Colour with point data:
https://beehive.pacificclimate.org/weather-anomaly-viewer
Updates Following Feedback: