-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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!
archmoj
left a comment
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
| .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'ANDfitboundsparts) and resolves #4154Demos:
fitboundson scoped maps withchoroplethtraces set with custom GeoJSONs via the newgeojsonattribute andfeatureidkeyfitboundsaround Brazil (locations: ['BRA']) for all projection typesIn brief,
choroplethandscattergeotraces (those that uselocationsas opposed tolonandlatcoordinates) similar to howchoroplethmapboxworks.choroplethmapbox, we use an attribute calledgeojsonto set custom GeoJSONs either as a JSON object or with a URL.locationmodevalue'geojson-id'to allow users to switch between custom GeoJSONs and our "stock"ISO-3orUSA-statesGeoJSONsfeatureidkeytochoroplethmapbox(as requested in Add new choroplethmapbox locationmode for geojsonproperties.id#4154) but also tochoroplethandscattergeotracesgeo.fitboundsan 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 forchoroplethtraces and lon/lat/centroid coordinates forscattergeotraces.'geojson'where the bounding box of the entire custom GeoJSON set in the traces'geojsonfield is consideredPotential TODOs:
[ ]Add specialfitboundslogic for conic projections. Scroll down to the newgeo_fitbounds-locationsmock to see the less-than-ideal behaviour. This may be as easy as fillingprojection.parallelsto 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/bboxpackage added in this PR with our own compute-bounding-box routine.@turf/bboxgives 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.visibleattribute similar toaxis.visibleinstead. Done in 0909f33Asking @archmoj to review !