Skip to content
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

GeoJSON defaults #2256

Merged
merged 8 commits into from Nov 7, 2014
Merged

GeoJSON defaults #2256

merged 8 commits into from Nov 7, 2014

Conversation

mramato
Copy link
Member

@mramato mramato commented Nov 5, 2014

NOTE: This builds on top of #2255, so review/merge that first

  • Added the ability to specify global GeoJSON default styling. This is done by introducing simplestyle-like static properties on GeoJsonDataSource.
  • Added an options parameter to GeoJsonDataSource.load, GeoJsonDataSource.loadUrl, and GeoJsonDataSource.fromUrl that takes the same set of parameters as above in order to allow basic per-instance styling.
  • Deprecated the sourceUri parameter to GeoJsonDatasource.load, use options.sourceUri instead.
  • Updated the GeoJSON & TopoJSON sandcastle example to show some default styling.
  • Refactored the entire GeoJSON test module to use waitsForPromise. Also added tests for the new functionality.

@mramato
Copy link
Member Author

mramato commented Nov 5, 2014

Fixes #2167

@@ -449,19 +475,156 @@ define([
* Creates a new instance and asynchronously loads the provided url.
*
* @param {Object} url The url to be processed.
* @param {Object} [options] An object with the following properties:
* @param {Number} [options.markerSize] The default size of the map pin created for each point, in pixels.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to list the defaults here I think.

@kring
Copy link
Member

kring commented Nov 6, 2014

Tests pass, Sandcastle example works nicely, and the code looks reasonable (admittedly I didn't look at the code in excruciating detail). So all good by me other than the one minor comment. I just reviewed this pull request as a whole instead of your other one separately. I'll give others a chance to review if they want before merging.

I'll probably use this feature in National Map right away, so thanks for doing it!

* Fixed a bug in `PolylineGeometry` that would cause the geometry to be split across the IDL for 3D only scenes. [#1197](https://github.com/AnalyticalGraphicsInc/cesium/issues/1197)
* Deprecated the `sourceUri` parameter to `GeoJsonDatasource.load`, use `options.sourceUri` instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in a Deprecated section at the top, I believe.

@mramato
Copy link
Member Author

mramato commented Nov 6, 2014

Okay, updates are in.

kring added a commit that referenced this pull request Nov 7, 2014
@kring kring merged commit 6d4048e into master Nov 7, 2014
@kring kring deleted the geojson-defaults branch November 7, 2014 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants