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 cleanup #1952

Merged
merged 9 commits into from
Jul 25, 2014
Merged

GeoJSON cleanup #1952

merged 9 commits into from
Jul 25, 2014

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jul 24, 2014

  1. Rewrite the GeoJsonDataSource Sandcastle example to show how to apply custom graphics. This will serve as a reference for a new DataSource tutorial I'll write sometime soon.
  2. Delete no longer used sample data.
  3. Removed defaultPoint, defaultLine, and defaultPolygon from GeoJsonDataSource because they just aren't very useful.
  4. Created a new CSS class cesium-infoBox-defaultTable that can be used by data sources to receive default infobox styling. Made GeoJsonDataSource use this class.
  5. Fixed several bugs I uncovered during this process and update specs to all pass after changes.

mramato added 4 commits July 23, 2014 15:41
1. Streamline Sandcastle example
2. Expose basic graphics objects instead of entire entities for default options
3. Add CZML Sandcastle example to the `DataSources` label.
@emackey
Copy link
Contributor

emackey commented Jul 24, 2014

Mind if I tweak the new table CSS?

@mramato
Copy link
Contributor Author

mramato commented Jul 24, 2014

I was counting on it.

@emackey
Copy link
Contributor

emackey commented Jul 24, 2014

Done

@mramato
Copy link
Contributor Author

mramato commented Jul 24, 2014

Feel free to review and merge it as well @emackey ;)

@emackey
Copy link
Contributor

emackey commented Jul 24, 2014

This needs your new Sandcastle.reset logic.

@emackey
Copy link
Contributor

emackey commented Jul 24, 2014

Nevermind, that's not in master yet. I'll merge this first and then you can fix it in #1956.

var key;
var nameProperty;
for (key in properties) {
if (properties.hasOwnProperty(key) && properties[key]) {
var upperKey = key.toUpperCase();
if (upperKey === 'NAME' || upperKey === 'TITLE') {
var upperKey = key.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Pointless nitpicking time. Should this be called lowerKey now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not pointless at all, thanks. I made this change because as a rule we always need to go to lower case because some Unicode characters upper case to the same thing.

@emackey
Copy link
Contributor

emackey commented Jul 24, 2014

Just 2 comments in the diffs.

@mramato
Copy link
Contributor Author

mramato commented Jul 24, 2014

ready

emackey added a commit that referenced this pull request Jul 25, 2014
@emackey emackey merged commit 71de214 into master Jul 25, 2014
@mramato mramato deleted the geojson branch July 25, 2014 13:26
@mramato mramato mentioned this pull request Jul 25, 2014
70 tasks
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.

2 participants