Skip to content

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

Merged
merged 12 commits into from
Dec 20, 2019

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Dec 11, 2019

Resolves #4267 (the 'geojson-id' AND fitbounds parts) and resolves #4154

Demos:


In brief,

  • 6397ab3 adds to ability to set "custom" GeoJSON features to choropleth and scattergeo traces (those that use locations as opposed to lon and lat coordinates) similar to how choroplethmapbox works.
    • Like for choroplethmapbox, we use an attribute called geojson to set custom GeoJSONs either as a JSON object or with a URL.
    • We also add a new locationmode value 'geojson-id' to allow users to switch between custom GeoJSONs and our "stock" ISO-3 or USA-states GeoJSONs
  • 5709d1b adds featureidkey to choroplethmapbox (as requested in Add new choroplethmapbox locationmode for geojson properties.id #4154) but also to choropleth and scattergeo traces
  • 02d6143 is a big one where we add geo.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 for choropleth traces and lon/lat/centroid coordinates for scattergeo traces.
    • 'geojson' where the bounding box of the entire custom GeoJSON set in the traces' geojson field is considered
    • ... more implementation details are in written down in the commit message.
  • The others commits are fixups of bugs I encountered while working on this.

Potential TODOs:

  • [ ] Add special fitbounds logic for conic projections. Scroll down to the new geo_fitbounds-locations mock to see the less-than-ideal behaviour. This may be as easy as filling projection.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 of v1.52.0. Differed to Replace @turf/bbox in geo fitbounds routine #4436
  • 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. Done in 0909f33

Asking @archmoj to review !

- 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!
Copy link
Contributor

@archmoj archmoj left a 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.

@@ -1390,6 +1555,8 @@ describe('Test geo interactions', function() {
.catch(failTest)
.then(done);
});

// TODO add test for scope:'none'
Copy link
Contributor

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?

Copy link
Contributor Author

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
@archmoj
Copy link
Contributor

archmoj commented Dec 20, 2019

All looks good to me.
@etpinard would you please updating the PR demos using the latest builds?

@etpinard
Copy link
Contributor Author

@archmoj this PR is ready for another round of review.

I decided to differ the "fitbounds + conic projection" item on the TODO list as well as the "replace @turf/bbox" item for later.

@etpinard
Copy link
Contributor Author

ould you please updating the PR demos using the latest builds?

Not that it matters much, but here there are:

@archmoj
Copy link
Contributor

archmoj commented Dec 20, 2019

Thanks very much.
Please 💃 💃 💃
After the test passed on CI.

@etpinard etpinard merged commit 1254e73 into master Dec 20, 2019
@etpinard etpinard deleted the locationmode-geojson-id branch December 20, 2019 20:20
@mbkupfer
Copy link

mbkupfer commented Jan 8, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
5 participants