-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Geo improvements: fitbounds, 'geojson-id' locationmode and 'featureidkey' #4419
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
Conversation
- move extractTraceFeature & fetchTraceGeoData logic from choroplethmapbox/convert.js -> geo_location_utils.js - move feature2polygons from choropleth/plot -> geo_location_utils.js - use lib/loggers instead of Lib in geo_location_utils
... to choroplethmapbox, scattergeo and choropleth traces. - Coerced only when `locationmode: 'geojson-id'`. To determine which key (or nested key) to use to identify eahc geojson feature. - Improve error messages in extractTraceFeature
... with values false (the default and current behaviour), `'locations'` and `'geojson'`. - add `_module.calcGeoJSON` method to the scattergeo and choropleth trace modules, use `@turf/bbox` to compute location bounding boxes (for now), call `calcGeoJSON` before during geo 'plot' before `updateProjection` - add mock axes to geo lonaxis and lataxis, use them reuse doAutorange - adjust projection settings using autorange values - clear attributes that get auto-filled via `fitbounds` during the geo layout defaults - add three mocks TODOs - find improvement for `@turf/bbox` - improve fitbounds behaviour for conic projections
... in choropleth and scattergeo traces
... which makes `%{ct}` available to hovertemplate strings!
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.
Brilliant work!
🎖️ 🏅 🥇
@etpinard please find my few comments below.
test/jasmine/tests/geo_test.js
Outdated
@@ -1390,6 +1555,8 @@ describe('Test geo interactions', function() { | |||
.catch(failTest) | |||
.then(done); | |||
}); | |||
|
|||
// TODO add test for scope:'none' |
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.
Would you be interested to add these tests in this PR?
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.
Yes, when I'll get to this TODO item
- Add some "scope-none" option for @nicolaskruchten, which would hide all the base layers using a single setting. Thinking about this in more details today, I think it would be better to add a geo.visible attribute similar to axis.visible instead.
- guard against empty subplot list in modebar button handlers - fix 'when' -> 'went' typo in error msg - guard against (potential) infinite fitbounds scales - improve `geojson` attr description - fixup `projection.rotation` fallback in an assertion wrapper
- which when set to false, overrides all the geo.show* defaults - add jasmine default test - add `geo.visible` to geo_featureidkey mock
All looks good to me. |
@archmoj this PR is ready for another round of review. I decided to differ the " |
Not that it matters much, but here there are: |
Thanks very much. |
Thanks for this update. Any chance there will be documentation on how this can be used in practice? All the codepen examples seem a bit contrived with all the coordinates being hardcoded and all. |
Resolves #4267 (the
'geojson-id'
ANDfitbounds
parts) and resolves #4154Demos:
fitbounds
on scoped maps withchoropleth
traces set with custom GeoJSONs via the newgeojson
attribute andfeatureidkey
fitbounds
around Brazil (locations: ['BRA']
) for all projection typesIn brief,
choropleth
andscattergeo
traces (those that uselocations
as opposed tolon
andlat
coordinates) similar to howchoroplethmapbox
works.choroplethmapbox
, we use an attribute calledgeojson
to set custom GeoJSONs either as a JSON object or with a URL.locationmode
value'geojson-id'
to allow users to switch between custom GeoJSONs and our "stock"ISO-3
orUSA-states
GeoJSONsfeatureidkey
tochoroplethmapbox
(as requested in Add new choroplethmapbox locationmode for geojsonproperties.id
#4154) but also tochoropleth
andscattergeo
tracesgeo.fitbounds
an enum with three possible values:false
, the default and current behaviour,'locations'
which means consider only the visible data in the fitbound computation, that is polygons forchoropleth
traces and lon/lat/centroid coordinates forscattergeo
traces.'geojson'
where the bounding box of the entire custom GeoJSON set in the traces'geojson
field is consideredPotential TODOs:
[ ]Add specialfitbounds
logic for conic projections. Scroll down to the newgeo_fitbounds-locations
mock to see the less-than-ideal behaviour. This may be as easy as fillingprojection.parallels
to the computed bounds' min and max latitudes, but I wonder if there's a formula out-there that gives the "optimal" parallels given a region of interest. Differed. I can't find a formula that works well for all fitbound polygons[ ]Replace@turf/bbox
package added in this PR with our own compute-bounding-box routine.@turf/bbox
gives the wrong bounds when a feature collections has polygons on either side of the anti-meridian (e.g. Alaska). Maybe we could leave this for later to not block the release ofv1.52.0
. Differed to Replace @turf/bbox in geo fitbounds routine #4436geo.visible
attribute similar toaxis.visible
instead. Done in 0909f33Asking @archmoj to review !