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

Allow custom geoJson description function #3140

Closed

Conversation

RacingTadpole
Copy link
Contributor

Some of our data sources have terse keys in their descriptions, which we would like to make more readable for our users (eg. at a minimum, by stripping out underscores).

To make this easier, I changed GeoJsonDataSource so we can customize how it wraps the properties up into an html description. The default behavior remains the same, but when calling load, you can specify the custom describe function in the options.

kring added 30 commits June 5, 2014 17:57
Conflicts:
	Source/Widgets/Viewer/Viewer.js
It wasn't including all of the necessary worker files in the build output.
Conflicts:
	Source/Widgets/Viewer/viewerEntityMixin.js
It tells us what triangle was picked, not just what position within the
triangle.
This breaks the normal Cesium setup.  I'm not sure yet the best way to
support both.
1.0 release

Conflicts:
	Source/Widgets/Timeline/Timeline.js
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 28, 2015

Thanks for the contribution. Can you add yourself to CONTRIBUTORS.md under NICTA?

@mramato is the best person to review. Bear with us as we're finishing up the 1.15 release for next week.

@RacingTadpole
Copy link
Contributor Author

Great, thanks @pjcozzi!

1.15 release

Conflicts:
	Source/Workers/cesiumWorkerBootstrapper.js
	gulpfile.js
	package.json
@@ -189,7 +189,7 @@ define([

var description = properties.description;
if (!defined(description)) {
entity.description = new CallbackProperty(createDescriptionCallback(properties, nameProperty), true);
entity.description = new CallbackProperty(createDescriptionCallback(describe, properties, nameProperty), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to have the provided describe return a Property instance and be called instead of creating a CallbackProperty every time? It's almost the same as you have now, but I think it would be more flexible. For example, the way this is currently set up, there's no good way (as far as I can tell) to supply an asynchronous describe function, but if you could return a custom property, that would be easy. What do you think?

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, that sounds more flexible - you could even gain flexibility over the nameProperty calculation then too.

I could change line 192 to read something like (with appropriate checks and balances):

entity.description = options.describeProperty(properties, nameProperty)

Then the user would define their custom describe function, and call load with options.describeProperty =

function(properties, nameProperty) {
    return new CallbackProperty(createDescriptionCallback(describe, properties, nameProperty), true);
}

Have I understood you right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's exactly what I had in mind. Also, due to the automatic wrapping/coercion we do for simple types, they can also return a single string back instead of a Property instance and everything will still work.

@mramato
Copy link
Contributor

mramato commented Nov 3, 2015

Just those couple comments and that one question.

@RacingTadpole
Copy link
Contributor Author

I've made these changes now.

Allow custom geoJson description function
@kring kring closed this Nov 5, 2015
@kring kring deleted the upstream_geojson_description branch November 5, 2015 00:52
@kring
Copy link
Member

kring commented Nov 5, 2015

Well I have no idea how to use Git and have made a mess of this pull request while trying to merge it into our fork. So @RacingTadpole opened a new, equivalent one, referenced above. Sorry guys!

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

9 participants