cmv / cmv-app

CMV - The Configurable Map Viewer - A community supported open source mapping framework built with the Esri JavaScript API and the Dojo Toolkit
https://demo.cmv.io/
MIT License
323 stars 278 forks source link

Pass in "customBasemaps" to Basemap Widget? #65

Closed gobsmack closed 10 years ago

gobsmack commented 10 years ago

I have an array of basemaps that I loaded from my config. Rather than editing the source of the Basemap widget, I'd like to pass in that array of basemaps. Is there a trick with Dojo Widgets where I can override that customBasemaps private member? Or, is it necessary to have a constructor parameter for this?

tmcgee commented 10 years ago

I'm also not a fan of customization inserted directly in the widget source.

The identify widget is an example of a slightly more app-specific customization. The Identify widget has a config file located at js/gis/dijit/Identify/config.js. This file included in the defines for the Identify widget as:

'./Identify/config'

To make this more generic, the change that I am considering is moving the identify config into the js/viewer folder as identify.js so the define would be modified to something like this:

'../viewer/Identify'

This moves the customization away from the widget source as well as out of the js/gis folider. This approach allows for a single gis folder (virtual directory in IIS) to be shared by multiple applications with app-specific customization confined to the viewer folder (or maybe a "config" folder?).

This doesn't directly answer your question about the basemap widget but the approach would be similar.

Hope this helps. These thoughts are my own. @DavidSpriggs may have a different answer. ;)

gobsmack commented 10 years ago

@tmcgee Identify is a great example of how we could accomplish this. However, like you, I'd prefer to let the @DavidSpriggs widgets remain untouched as much as possible. I don't have a shared gis folder, but I use bower to deploy the freshest possible widget from GitHub. By contrast, I think it's OK to diverge in Controller.js, because that's really expected.

Your solution is the least disruptive, but it still leaves a hardcoded dependency to that configuration file. How about we load the configuration via AMD in Controller.js. Then, we pass that object into the Constructor of Basemaps?

That being said, there's another, even more disruptive, solution. Why not move all the baseMap config to default.js? Do something similar to the webmap spec:

baseMap: {
   baseMapMode: 'custom',
   mapStartBaseMap: 'street',
   baseMapLayers: [
      street: {
         title: 'Streets',
         basemap: new Basemap({
            id: 'street',
            layers: [new BasemapLayer({
               url: 'http://services.arcgisonline.com/ArcGIS/rest/services/World_Street_Map/MapServer'
            })]
         })
      },
      // etc etc from existing configuration
   ]
},
operationalLayers: [
   // etc etc etc
}

This would be my preferred approach. Configuration is loaded into Controller.js, and passed into the constructor of Basemaps. Or, I can choose any basemap Widget (Spriggs or other), and give it this configuration.

tmcgee commented 10 years ago

@gobsmack,

I assume you mean config.js not default.js.

That is a better approach. After writing my comment above, I realized I am already doing something similar with a "Find" widget I've been working on (based on the initial work of tr3vorm in issue #54) :

In config.js:

find: {
    id: 'sidebar_Find',
    include: true,
    title: 'Search',
    open: true,
    position: 0,
    queries: [
        {
            id: 0,
            name: 'Find A Map By Name',
            url: 'http://server/arcgis/rest/services/Parcelviewer/Flatfiles_AGS/MapServer',
            textSearchLabel: 'Name',
            textSearchHint: 'Enter the first few characters of the map\'s name',
            layerIds: [0,1,2,3],
            searchFields: ['NAME', 'PUDNAME', 'MAP_TITLE_']
        },
        {
            id: 1,
            name: 'Find A Parcel By APN',
            url: 'http://server/arcgis/rest/services/BaseMaps/Basemap_WM/MapServer',
            textSearchLabel: 'APN',
            textSearchHint: 'Enter the first few characters of the APN',
            layerIds: [9],
            searchFields: ['APN'],
            minChars: 5,
            hideGrid: true
        }
    ],
    outputSpatialReference: 4326,
    symbols: {
        point: { // creates a circle
            size: 10,
            lineColor: [0, 255, 255],
            lineWidth: 3,
            fillColor: [0, 255, 255, 1.0]
        },
        line: {
            color: [0, 255, 255],
            width: 3
        },
        polygon: {
            lineColor: [0, 255, 255],
            lineWidth: 4,
            fillColor: [255, 255, 255, 0]
        }
    }
},

In Controller.js:

    find: function (widgetConfig, position) {
        var findTP = this._createTitlePaneWidget(widgetConfig.id, widgetConfig.title, position, widgetConfig.open);
        require(['gis/dijit/Find'], lang.hitch(this, function (Find) {
            this.find = new Find({
                map: this.map,
                queries: widgetConfig.queries,
                outputSpatialReference: widgetConfig.outputSpatialReference,
                symbols: widgetConfig.symbols
            }, domConstruct.create('div')).placeAt(findTP.containerNode);
            this.find.startup();
        }));
    },

Always looking to make things more generic and more configurable, I also have an approach in the works to add or replace widget loaders from the config.js. This allows a custom widget loader like find above or Larry Stout's PrintPlus to be added to the config and the Controller remains untouched. That's perhaps a topic for another day...

gobsmack commented 10 years ago

So, I made this change in my own code, and it's working great. The config.js was actually a little different than I though. It looks like this:

baseMap: {
   basemapMode: 'custom',
   mapStartBasemap: 'street',
   basemapsToShow: ['streets', 'satellite', 'hybrid', 'topo', 'gray', 'oceans', 'national-geographic', 'osm'],  
   customBasemaps: {
      street: {
         title: 'Streets',
         basemap: new Basemap({
            id: 'street',
            layers: [new BasemapLayer({
               url: 'http://services.arcgisonline.com/ArcGIS/rest/services/World_Street_Map/MapServer'
            })]
         })
      },
      // etc etc
   },
   agolBasemaps: {
      streets: {
    title: 'Streets'
      },
      // etc etc
   },
},
operationalLayers: [
]

Then, in Controller.js, I made this change:

this.basemaps = new Basemaps({
   map: this.map,
   mode: this.config.baseMap.basemapMode,
   title: 'Basemaps',
   mapStartBasemap: this.config.baseMap.mapStartBasemap,
   basemapsToShow: this.config.baseMap.basemapsToShow,
   agolBasemaps: this.config.baseMap.agolBasemaps,
   customBasemaps: this.config.baseMap.customBasemaps,
}, 'basemapsDijit');

After that, in the widget, I just had to switch to:

this.agolBasemaps
this.customBasemaps

I won't go through a pull request until we hear from @DavidSpriggs about the idea, in case it doesn't align with his goals.

@tmcgee Along the same lines, I think a LayerLoader would be great, so the code to load the layers can be outside of the Controller. Even after I forked the Controller, David has added all this great new conditional AMD-require stuff, that's not in my controller. Maybe we can get a new folder called "tools" that would contain non-UI widgets (WidgetLoader, LayerLoader).

tmcgee commented 10 years ago

your mods for baseMaps look good to me. ;)

"conditional AMD-require stuff" - I added that code in a recent PR and so will give some thought as to where that fits in.

Along the lines of a "tools" folder, I've considered moving config.js to a "custom" or "config" folder. The Find widget I referred to earlier resides in the widgets folder under custom (custom/widgets).

All this discussion makes syncing up with David's core code easier moving forward. good stuff.

DavidSpriggs commented 10 years ago

@gobsmack I like the idea and It looks good. Please submit a PR. I wrote the basemap widget long ago, before there was any clear strategy, so any improvements are welcome.

tr3vorm commented 10 years ago

I merged the most recent changes, and now I am confused as to how custom basemaps are meant to be configured. I have it all working OK, except that I still have to define a default agol basemap. When the viewer loads, the agol basemap is drawn first, followed by my default custom basemap. Is this the expected behavior?

tmcgee commented 10 years ago

though I am 'agol' mode for basemaps, I noticed similar behavior of the basemap drawing twice - the default and then my desired basemap.

What is the value of the basemap for the mapOptions config object? It should be the same as the value of the "mapStartBasemap" in the config object for the basemaps widget. At least that is what worked for me.

There is also a comment in Controller.js that says:

// issue to fix: if using custom basemap, you need to load the basemap widget now or map::load will never fire

// this.basemaps = new Basemaps({
//     map: this.map,
//     mode: config.basemapMode,
//     title: 'Basemaps',
//     mapStartBasemap: config.mapStartBasemap,
//     basemapsToShow: config.basemapsToShow
// }, 'basemapsDijit');
// this.basemaps.startup();

That does not quite sound related to your issue but figured I'd point it out just in case.

Hope this helps.

tr3vorm commented 10 years ago

Thanks - I have changed to agol, and set just set the default to the same as desired for now.

DavidSpriggs commented 10 years ago

@tmcgee @tr3vorm Yes this is a new issue that was introduced by moving the basemap widget out as an optional widget. The way the map object is created, it needs to have a basemap string or you have to create and add your own custom map layer if you don't set a basemap string. The basemap widget handled this all before but now that it's optional we need to rework this. I have not had time to deal with it yet. Probably a simple check to see if you have a basemap string and if not create the basemap widget.

friendde commented 10 years ago

I will try to help with this issue as I have a related problem I want to solve. My problem is the Class Map :: maxZoom scale. See Constructor Details section https://developers.arcgis.com/javascript/jsapi/map-amd.html for maxZoom, excerpt below. My custom basemaps are dynamic services without predefined zoom levels.

...maximum zoom level will be calculated based on maxScale or the maximum zoom level supported by the basemap. ....

Even though I load my own custom basemaps I think the Parameter maxZoom for my custom basemap is limited by the first basemap that is loaded, in my case it is the ESRI streets map. I have to load an agol map first just as @tr3vorm and @tmcgee mentioned above. Since I can't first load my own custom basemap I don't how to test my theory.

I tried forcing maxScale and maxZoom in Viewer.js under mapOptions{} I was wondering if the basemap widget does not update the maxScale of the new basemap when changing from a agol map to a custom basemap.

Does the agol parameter for maxScale persist after basemap.startup even when swicthing to a new basemap?

friendde commented 10 years ago

I got my custom basemap to load first. This solved my maxZoom scale problem.

Using basemapMode: 'custom' in Viewer.js then in Controller.js use an IF statement to check for agol or custom, then load custom basemap if true.

But now the BasemapGallery will not populate with my custom basemaps. I know it has to do with basemapsToShow: configViewer.basemapsToShow in Controller.js and I don't know how to fix it.

//Viewer.js - note that I renamed Viewer.js to configViewer.js throughout the code
    return {
        //default mapClick mode, mapClickMode lets widgets know what mode the map is in to avoid multipult map click actions from taking place (ie identify while drawing).
        defaultMapClickMode: 'identify',
        // basemapMode: must be either "agol" or "custom"
        basemapMode: 'custom',
        // map options, passed to map constructor. see: https://developers.arcgis.com/javascript/jsapi/map-amd.html#map1
        mapOptions: {
            basemap: 'streets',
            //maxZoom: -1,
            center: [-82.323, 29.660],
            zoom: 14,
            sliderStyle: 'small',
            logo: false,
        },
// Controller.js - added 'gis/dijit/Basemaps' to define(), function()
        initMap: function() {
            if (configViewer.basemapMode == 'custom'){
                console.log('configViewer.basemapMode is custom')
                this.map = new Map('map');
                this.basemaps = new Basemaps({
                 map: this.map,
                 mode: configViewer.basemapMode,
                 title: 'Basemaps',
                 mapStartBasemap: configViewer.mapStartBasemap,
                 basemapsToShow: configViewer.basemapsToShow
             }, 'basemapsDijit');
             this.basemaps.startup();
            } else {
                this.map = new Map('map', configViewer.mapOptions);
            }

            this.map.on('load', lang.hitch(this, 'initLayers'));
            this.map.on('layers-add-result', lang.hitch(this, 'initWidgets'));
//Basemaps.js - modified mode: and mapStartBasemap: to use Viewer.basemapMpde and Viewer.MapOptions.basemap

 // main basemap widget
    return declare([_WidgetBase, _TemplatedMixin, _WidgetsInTemplateMixin], {
        templateString: template,
        widgetsInTemplate: true,
        mode: configViewer.basemapMode, //'custom',
        title: 'Basemaps',
        //baseClass: 'gis_Basemaps_Dijit',
        //buttonClass: 'gis_Basemaps_Button',
        //menuClass: 'gis_Basemaps_Menu',
        mapStartBasemap: configViewer.mapOptions.basemap, //'navigatorBaseMap',
        //basemapsToShow: ['streets', 'satellite', 'hybrid', 'topo', 'gray', 'oceans', 'national-geographic', 'osm'],
        validBasemaps: [],
        postCreate: function() {
            this.inherited(arguments);
            this.currentBasemap = this.mapStartBasemap || null;
            console.log('currentBasemap: ' + this.currentBasemap)

            if (this.mode === 'custom') {
                console.log('basemapMode: ' + this.mode)
                this.gallery = new BasemapGallery({
                    map: this.map,
                    showArcGISBasemaps: false,
                    basemaps: functional.map(customBasemaps, function(map) {
                        return map.basemap;
                    })
                });
                this.gallery.select(this.mapStartBasemap);
                this.gallery.startup();
            }
tmcgee commented 10 years ago

Instead of tweaking the Controller code, have you tried loading your initial custom basemap via the mapOptions in your config? Something like:

mapOptions: {
    basemap: new Basemap({
        id: 'my-custom-basemap',
        layers: [new BasemapLayer({
            url: 'http://services.arcgisonline.com/ArcGIS/rest/services/World_Street_Map/MapServer'
        })]
    }),
    center: [-96.59179687497497, 39.09596293629694],
    zoom: 5,
    sliderStyle: 'small'
},

I did not test this too vigorously.

tmcgee commented 10 years ago

@tr3vorm might this solve your issue as well?

green3g commented 10 years ago
mapOptions: {
    basemap: new Basemap({
        id: 'my-custom-basemap',
        layers: [new BasemapLayer({
            url: 'http://services.arcgisonline.com/ArcGIS/rest/services/World_Street_Map/MapServer'
        })]
    }),
    center: [-96.59179687497497, 39.09596293629694],
    zoom: 5,
    sliderStyle: 'small'
},

This solved my issue. Although is it bad practice to have this exact same code in the baselayers.js?

friendde commented 10 years ago

@tmcgee The snippet you provided will load an agol basemap and the BasemapGallery with my custom maps. But if I substitute my map service in your snippet then it fails to load at all. The console seems to indicate it is looking for a tiled map service. Strange to me because that error does not appear when I use my changes above yours.

And if I use yours with the sample server address then I still have limited maxZoom scale.

---- Console Log ---- SyntaxError: Using //@ to indicate sourceMappingURL pragmas is deprecated. Use //# instead main.js:48 Use of getAttributeNode() is deprecated. Use getAttribute() instead. init.js:149 Use of attributes' specified attribute is deprecated. It always returns true. init.js:149 "baseMap: my-custom-basemap" Controller.js:61 TypeError: p is undefined Stack trace: .cache["esri/layers/TiledMapServiceLayer"]/</d<._setMap@http://ServerName/arcgis_js_api/library/3.9/3.9/init.js:1465

green3g commented 10 years ago

I think you are correct, it has to be a tiled map service provided for the mapOptions object. We are using a tiled map service, and when I switched the BasemapLayer to a dynamic service, I got the same error.

Although, I am able to use dynamic map services in the BasemapLayers in the Basemap.js config.

tmcgee commented 10 years ago

@tr3vorm Perhaps it is bad practice but only a little. ;)

@friendde I have not worked with non-tiled basemaps - only as operational layers. Here's a possibility:

In the JS API, it seems like you must have an initial tiled basemap or you can pass no basemap at all basemap: null. In this second case, I believe the map gets the stuff like spatialReference and maxZoom from the operational layers. So, when you add the BaseMap Widget immediately after loading a map with no base map defined (as in your first example today), it loads your custom basemaps in essence as if they were operational layers and so the information such as maxZoom just happens to be correct. Hope that makes some sense.

I know this does not completely explain the missing basemaps in the dropdown list but it might be leading to an ultimate answer there.

friendde commented 10 years ago

@tmcgee it did return NULL so I agree the API defaulted to a tiledmapservice expectation. We don't use tiledmap services; lack of storage space. Our dynamic layers work fine and fast enough. Seeing that the Basemap widget allows for dynamic basemaps I am sure we can get overcome this issue.

@roemhildtg It is magical what David did with the Basemap widget. Somehow he got around this issue.

Interesting side effect appeared, the Find widget is broken with my own changes. More specifically the findTask runs as expected but the highlighting and zoom-to results fails.

Now that my basemap is in 102660, not 4326 as before, this error appears in the console - "Map: Geometry (wkid: 4326) cannot be converted to spatial reference of the map (wkid: 102660)"

This is a similar error that @msereda opened in issue #79. I had suspected it had to do with the basemap(s). I sent David a link to our map services by email, when he has time I assume he will test out with wkid: 102660.

Anyway, just posting the newly broken Find widget here as an FYI for the group in case you do change basemap wkid. I am not going to open a new issue as it is my problem at the moment.

tmcgee commented 10 years ago

What happens when you change the outputSpatialReference for the find widget to 102660? You can pass that in the options, otherwise it uses a default of 4326. Note that zooming to points might have an issue because of the units.

tmcgee commented 10 years ago

Note that we've gotten way, way off track from the original issue so these new observations should go into a new issue.

friendde commented 10 years ago

I agree we are off track. I will explore the new problem on my own. If I get stuck I will open a new issue. My previous post was FYI only.

For now I will continue solving the basemap issue discussed here. A breakthrough is eminent.

DavidSpriggs commented 10 years ago

Are we ready to make this happen again? All pending PR's are merged. Submit a new PR when ready.

tmcgee commented 10 years ago

Yes we are ready IMHO. I am going to be away from the office this morning and so will merge all your recent changes and submit the PR hopefully this afternoon.

tmcgee commented 10 years ago

PR #104 Submitted. This PR addresses the original request of passing in custom maps to the basemap widget. It does not change any logic regarding tiled vs non-tiled basemaps or non-mercator basemaps.

tmcgee commented 10 years ago

The original issue has been addressed. Can this be closed?

DavidSpriggs commented 10 years ago

Issue resolved. in #104