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

make identify listen for new layer contol layer adds #648

Merged
merged 2 commits into from Dec 18, 2016

Conversation

Projects
None yet
3 participants
@roemhildtg
Copy link
Member

roemhildtg commented Dec 8, 2016

Moved the layerinfos init code into its own function initLayerInfos so we can call this when the topic is published that adds new layers to the layer control.

One example of using this would be in a custom layer generating widget:

                layer = new FeatureLayer(serviceURL, layerOptions);
                this._labelLayers[layerId] = {
                    layer: layer,
                    iconNode: this.activeLayer.iconNode
                };
                this.map.addLayer(layer);

                // notify layer control
                // wait for async layer loads
                layer.on('load', lang.hitch(this, function() {
                    topic.publish('identify/addLayerInfos', [{
                        type: 'feature',
                        layer: layer,
                        title: title
                    }]);
                }))

I realize this is using a layerControl topic but it makes more sense to use the existing topic than register a new one just for identify widget. It seems silly to have any widget that adds a layer publish two topics (or more) if we're just adding a new layer to the app.

Perhaps we should make a more generic layer adding topic.

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 9, 2016

The restructure is good and makes sense. I'd prefer to keep the topics separate. The separate topic makes more sense to me because of the situation when there is no layerControl included in the application. Perhaps the layerControl could optionally publish the identify topic when layers are added?

Curious if this is only relevant to dynamic layers? For feature layers, the layer's infoTemplate config would (or should) handle the identify. For that matter, the existence of an infoTemplates array maybe the appropriate trigger for the layerControl to publish the identify topic? Haven't studied the flow of this. Just thinking out loud to myself.

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Dec 9, 2016

It didn't look like the identify worked automatically on feature layers either. The layer.infoTemplate property was undefined. For the label layer example I linked, I needed to publish the topic in order for identify to work correctly.

@roemhildtg roemhildtg force-pushed the roemhildtg:identify-layer-add-topic branch from fd83e04 to 1d43481 Dec 9, 2016

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Dec 9, 2016

Not sure what happened in the build, but its updated.

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 12, 2016

I am not sure why the travis CI exited with this error Warning: Task "eslint" not found. Use --force to continue. I think it is a one time glitch due to travis timing. From what I just read, I believe that @DavidSpriggs as repo owner is the only one who can restart the build here. Or perhaps if you make another commit in the branch.

@DavidSpriggs

This comment has been minimized.

Copy link
Member

DavidSpriggs commented Dec 12, 2016

I restarted the build and it failed again:
screen shot 2016-12-12 at 10 20 59 am

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 12, 2016

thanks @DavidSpriggs at least now @roemhildtg knows what needs to be addressed.

@roemhildtg roemhildtg force-pushed the roemhildtg:identify-layer-add-topic branch from 1356e67 to 4c2a037 Dec 12, 2016

@DavidSpriggs DavidSpriggs requested a review from tmcgee Dec 13, 2016

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 13, 2016

@roemhildtg want to follow up regarding your comment:

It didn't look like the identify worked automatically on feature layers either. The layer.infoTemplate property was undefined.

When I use the DnD widget to add a csv or a kml file, the graphics on the resulting Feature Layer can be identified. This is with or without the Identify widget. That widget creates a generic infoTemplate for the feature layer. In your layer label example, there is no infoTemplate in the layerOptions when the feature layer is added to the map. Are you doing that elsewhere?

@tmcgee tmcgee added this to the v2.0.0-beta.1 milestone Dec 13, 2016

@tmcgee tmcgee added the enhancement label Dec 13, 2016

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Dec 13, 2016

@tmcgee, nope I wasn't doing that anywhere. I guess I hadn't looked into it enough, it looks like its as simple as adding this to the layer options?

infoTemplate: new InfoTemplate(null, '${*}'),

Edit:

It looks like this is the result of adding an empty info template:
image

So the approach to use identify widget to create the infotemplate seems like a more useful method. Thoughts?

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 15, 2016

@roemhildtg The simple template you show is the easiest way. You can also provide title, description, fieldInfos, mediaInfos, etc in the infoTemplate as the feature layer is added to the map. Bottom line is there are several ways to accomplish this and being able to mix and match multiple approaches is always good. We are not forcing any dependence on the Identify widget since a feature layer can be identified without it.

One reason I thought about for considering including the feature layer's info template in identify.js is to use the new custom formatters though I don't think those work for feature layers in our current implementation. That is worth including in the discussion of #650

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 15, 2016

Another enhancement that comes to mind is for the Identify widget to listen to the layer-add map event and react to any existing infoTemplate (feature layer) or infoTemplates array (dynamic layer). Following this Esri approach to defining templates might add increased compatibility going forward with web maps and possibly v4 of the API. Just a thought to capture while I thought of it.

EDIT: Hmmm. Maybe this listener is a better approach than using a topic? Thoughts?

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Dec 15, 2016

Here's a use case I'd like to discuss. I have a widget that creates feature layers based off of dynamic layers. I'd like to configure the identify for each layer that gets added, but I don't want to duplicate the configure-ability (is that a word?) of the identify widget so I'd rather just let the identify widget create the infotemplate.

Either the layer-add event or the topic would work. So if we use the layer-add event, the identify widget would look for existing info templates, and if it finds none, it would automatically create one?

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 18, 2016

Yes, the identify widget should use the existing infoTemplate if it exists or create a default one from the fields in the layer/sublayer.

Unless I am misunderstanding, I'm not sure that any of this addresses your desire for feature layers and dynamic layers to use the same infoTemplate defined only once. Maybe that isn't what you are asking. Each feature layer would be required to have a unique layer id when added to the map and so the index into the identify widget's infoTemplates for one feature layer would not be the same as the dynamic layer or another feature layer. So whether you define the infoTemplate directly in the feature layer or in the identify.js, each one must be separate.

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Dec 18, 2016

Will merge this as is. A topic to explicitly add a layer is good. A listener for the layer-add map event can be considered later.

@tmcgee tmcgee merged commit cfd44b2 into cmv:develop Dec 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@roemhildtg roemhildtg deleted the roemhildtg:identify-layer-add-topic branch Dec 18, 2016

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Dec 18, 2016

@tmcgee This addresses my use case because I can create an info template in my identify config, with the other info templates. Its nice to have that all in one place. In addition, I don't want to duplicate the functionality that the Identify widget provides.

Lets say my dynamic layer is with id: 'assets' and I create a feature layer with id: 'assets_12' which is the 12th layer in my dynamic layer. In my identify config I can do something like this:

var assets12 = {
    fieldInfos: [],
    //other stuff
}
assets: {
    12: assets12
},
assets_12: {
    12: assets12
}

and all of my identify popup info is stored in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment