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

Add setter for GeoJsonDataSource.name #5656

Merged
merged 2 commits into from
Jul 20, 2017
Merged

Add setter for GeoJsonDataSource.name #5656

merged 2 commits into from
Jul 20, 2017

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jul 19, 2017

Fixes #5653

@hpinkos
Copy link
Contributor Author

hpinkos commented Jul 19, 2017

@mramato do you have any idea why CI is failing? It's doing that on a couple of my branches

@@ -690,6 +690,12 @@ define([
name : {
get : function() {
return this._name;
},
set : function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably update the comment above to indicate the name can be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, thanks! I just pushed an update to the doc

Copy link
Contributor

Choose a reason for hiding this comment

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

I need to double check in the code, but I don't think we want to raise the _changed event, here. I think that event is meant for underlying Entity changes, which would basically cause more work to be done than needed if we raise it here. But like I said, I need to check how we are using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from CustomDataSource FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, looking at the code we don't seem to use this internally other than setting the DataSource clock in viewer, so this is fine as is.

@mramato mramato merged commit 96ab483 into master Jul 20, 2017
@mramato mramato deleted the geojson-name-setter branch July 20, 2017 14:25
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.

3 participants