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 the ability to add custom field formatters #618

Merged
merged 6 commits into from Oct 10, 2016

Conversation

Projects
None yet
2 participants
@roemhildtg
Copy link
Member

roemhildtg commented Oct 10, 2016

Just a proposal...comments?

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

I like it. Very simple and elegant solution.

@roemhildtg roemhildtg force-pushed the roemhildtg:proposal-custom-identify-formatter branch from 3b38792 to 86064a2 Oct 10, 2016

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

@roemhildtg Taking this one step further, there's another opportunity to include basic formatters when no infoTemplate exists in the widget configuration and so a default template is created. How about adding something here in the addDefaultFieldInfo method like what I did for the building default columns in the buildColumns method of the AttributeTable widget?

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 10, 2016

Okay, that would be a good idea. Also, on my own app I was working on improving the default template a bit. Basically, I wanted to include mediaInfos, but not override the default template fieldInfos and title that the Identify widget creates. Would this be appropriate to include in this enhancement?

        getInfoTemplate: function (layer, layerId, result) {
            var popup, config;
            if (result) {
                layerId = result.layerId;
            } else if (layerId === null) {
                layerId = layer.layerId;
            }

            var ids = this.identifies;
            if (ids.hasOwnProperty(layer.id)) {
                if (ids[layer.id].hasOwnProperty(layerId)) {
                    popup = ids[layer.id][layerId];
                    if (popup instanceof PopupTemplate) {
                        return popup;
                    }
                }
            } else {
                ids[layer.id] = {};
            }

            // by mixin in the users config with the default props we can
            // generate a config object that provides the basics automatically
            // while letting the user override only the parts they want...like mediaInfos
            config = lang.mixin(this.createDefaultInfoTemplate(layer, layerId, result), ids[layer.id][layerId] || {});

            popup = ids[layer.id][layerId] = new PopupTemplate(config);
            if (config.content) {
                popup.setContent(config.content);
            }

            return ids[layer.id][layerId];
        },

        createDefaultInfoTemplate: function (layer, layerId, result) {
            var popup = null,
                fieldInfos = [];

            var layerName = this.getLayerName(layer);
            if (result) {
                layerName = result.layerName;
            }

            // from the results
            if (result && result.feature) {
                var attributes = result.feature.attributes;
                if (attributes) {
                    for (var prop in attributes) {
                        if (attributes.hasOwnProperty(prop)) {
                            this.addDefaultFieldInfo(fieldInfos, {
                                fieldName: prop,
                                label: this.makeSentenceCase(prop),
                                visible: true
                            });
                        }
                    }
                }

                // from the outFields of the layer
            } else if (layer._outFields && (layer._outFields.length) && (layer._outFields[0] !== '*')) {

                var fields = layer.fields;
                array.forEach(layer._outFields, lang.hitch(this, function (fieldName) {
                    var foundField = array.filter(fields, function (field) {
                        return (field.name === fieldName);
                    });
                    if (foundField.length > 0) {
                        this.addDefaultFieldInfo(fieldInfos, {
                            fieldName: foundField[0].name,
                            label: foundField[0].alias,
                            visible: true
                        });
                    }
                }));

                // from the fields layer
            } else if (layer.fields) {

                array.forEach(layer.fields, lang.hitch(this, function (field) {
                    this.addDefaultFieldInfo(fieldInfos, {
                        fieldName: field.name,
                        label: field.alias === field.name ? this.makeSentenceCase(field.name) : field.alias,
                        visible: true
                    });
                }));
            }

            if (fieldInfos.length > 0) {
                popup = {
                    title: layerName,
                    fieldInfos: fieldInfos,
                    showAttachments: (layer.hasAttachments)
                };
            }

            return popup;
        },
        /**
         * converts a string to a nice sentence case format
         * @url http://stackoverflow.com/questions/196972/convert-string-to-title-case-with-javascript
         * @param  {string} str The string to convert
         * @return {string}     The converted string
         */
        makeSentenceCase: function (str) {
            if (!str.length) {
                return '';
            }
            str = str.toLowerCase().replace(/_/g, ' ').split(' ');
            for (var i = 0; i < str.length; i++) {
                str[i] = str[i].charAt(0).toUpperCase() + (str[i].substr(1).length ? str[i].substr(1) : '');
            }
            return (str.length ? str.join(' ') : str);
        }
@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

I like the mixin approach and the makeSentenceCase formatter method. However, you might get unexpected results if you include a carefully constructed description property in your template and then the createDefaultInfoTemplate method adds a fieldInfos array to the template.

@tmcgee tmcgee added this to the v2.0.0-beta.1 milestone Oct 10, 2016

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 10, 2016

Looking into the default formatters a bit... this line https://github.com/cmv/cmv-app/blob/develop/viewer/js/gis/dijit/Identify.js#L386 it seems always is getting evaluated. The other two checks are skipped which provides the esriFieldType info. Maybe the order should be:

  1. layer._outfields
  2. layer.fields
  3. result.feature.attributes

?

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

That line is only evaluated when there are results. The method is called here and here without the results argument.

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 10, 2016

Okay...so I'm realizing that graphics layers are handled internally by the api, not sure if there's a way to format the feature before its displayed in the popup.

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

that is correct.

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 10, 2016

The other problem is that dynamic layers don't have a fields or _outfields property, so they will not be able to get default formatters added since there's no field type info available.

We could fetch each layer's metadata...but that would be an extra request and probably require a lot of extra work in how the identify is accomplished.

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

I have considered fetching the layer metadata and you're right, it is a lot of work with overhead.

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 10, 2016

However, you might get unexpected results if you include a carefully constructed description property in your template and then the createDefaultInfoTemplate method adds a fieldInfos array to the template.

I tried it out with the cmv config and it looks like the description is still working. I think the description automatically overrides the fieldInfos in the esri api.

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

@roemhildtg My mention of description comes from comments made by @kcarrier In issue #177. That's a lengthy discussion In his comments on August 19, 2015, the order of description and fieldInfos seemed to make a difference. The context in this case is a little different so it may not be relevant. Just worth noting.

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 10, 2016

Good to know. I can separate out that functionality into a new pr for additional testing. Your call.

@@ -313,11 +327,21 @@ define([
return;
}
}
fSet.push(result.feature);
var feature = this.getFormattedFeature(result.feature);
// console.log(feature);

This comment has been minimized.

@tmcgee

tmcgee Oct 10, 2016

Member

obviously it won't affect anything but it would be good to remove this commented line.

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 10, 2016

The mixin functionality isn't included in this PR so it won't affect the addition of the formatters. We can consider that separately.

EDIT: sorry, the mixin functionality is there. Let's pull that out into a separate PR.

EDIT 2: Scratch that. That may just be a corner case. Let's keep it and have more testing during the beta period of v 2.0.

@tmcgee tmcgee merged commit 0b3838b into cmv:develop Oct 10, 2016

1 check passed

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

@tmcgee tmcgee removed the proposal label Oct 10, 2016

@roemhildtg roemhildtg deleted the roemhildtg:proposal-custom-identify-formatter branch Nov 1, 2016

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