Skip to content
This repository has been archived by the owner on Dec 1, 2020. It is now read-only.

Add "esriBasemap" directive #121

Merged
merged 5 commits into from
Oct 12, 2015
Merged

Add "esriBasemap" directive #121

merged 5 commits into from
Oct 12, 2015

Conversation

Kollibri
Copy link
Contributor

@Kollibri Kollibri commented Oct 7, 2015

Add an "esriBasemap" directive that can be used to add custom basemaps to the map.

@Kollibri
Copy link
Contributor Author

Kollibri commented Oct 7, 2015

An example use of this:

<esri-map id="interactiveMap" map-options="map.options">
<esri-basemap urls="http://urlToBaseMap/MapServer"
                name="customStreet"
                title="Custom Street Map"
                thumbnailurl="">
</esri-basemap>
<esri-basemap urls="http://urlToBaseMap/MapServer"
                name="customStreetTpt"
                title="Custom Street Map Transparent"
                thumbnailurl="">
</esri-basemap>
</esri-map>

(function (angular) {
'use strict';

angular.module('esri.map').directive('esriBasemap', function (esriRegistry) {
Copy link
Member

Choose a reason for hiding this comment

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

suggest you remove esriRegistry, it does not appear to be used.

@tomwayson
Copy link
Member

@Kollibri

Thanks for this. I think this will provide value, but I've made a few suggestions that I'd like to see before merging. Pretty much, try to follow the style I did in the info template directive.

If you can make those changes I will merge.

@Kollibri
Copy link
Contributor Author

@tomwayson

Hi. I have tried making the changes you suggested. But running into few problems when it comes to making the basemap addition into a function of the map controller. Basically, I believe the basemaps have to be set before the map is initialized. I don't think you can add basemaps to a map that is already created (I could be wrong about this, but I think this is the way it works based on the JS API documentation here: https://developers.arcgis.com/javascript/jsapi/esri.basemaps-amd.html).
Since the basemaps have to be defined before the map is initialized, I believe they have to be defined within the "require" function that has all the other mapOptions being initialized at the same time.

Please correct me if wrong. I will make the other changes to esriBasemap (the unneeded controller references, etc).

@tomwayson tomwayson merged commit c8a1dcb into Esri:master Oct 12, 2015
@tomwayson
Copy link
Member

Thanks @Kollibri.

I've merged your changes and added a test page and added the directive to the gulp build.

I will also likely change this to an attribute that you place on the map (instead of a child directive) like custom-basemaps. The reason is that you are right about the timing of when the custom basemap is added. Directive controllers are instantiated from top down and then their link functions are called bottom up, which means this should not be working at all right now b/c the map's controller (which checks for custom basemap definitions) executes before the basemap directive's link function (which adds the custom basemap definition). The only reason it works is b/c the code in the map's controller executes w/in an async require() block, which is basically a fluke. I'd prefer not to rely on the async timing.

I think by adding the custom basemaps as an attribute on the map, they will be available whenever the map is initialized.

The only other consideration that I'm thinking of is that changing these basemaps means affects more than just the current map, so maybe it's best to make this change somewhere that reflects the "global" nature of the change (like a service to represent esri/basemaps and possibly esri/config?).

Anyway, my point is, I've merged the changes, but plan to change the API, so use w/ caution.

Thanks again for your contributions.

@tomwayson
Copy link
Member

@jwasil would love your thoughts on custom-basemaps map attribute vs a service to represent esri/basemaps.

@jwasilgeo
Copy link
Contributor

@Kollibri cool stuff, thanks for the ongoing contributions!

@tomwayson, I could see it going both ways as an attribute or as a new service. If I follow, I suppose the +'s of it being some kind of service is that it can have its own space to live in rather than continuing to add more complex/verbose items being directly to another esriMap attribute. At the moment I am leaning towards that, but I'm sure I'm not seeing all the different considerations to take into account.

@tomwayson
Copy link
Member

I think my main concern w/ having it be an attribute of esriMap is that you are making changes to something that will affect all maps w/in the app, not just the current map - that's not too big a deal in this case b/c we're only adding basemap definitions (not deleting them).

I guess, b/c I'm refactoring all code to the service(s) anyway, this could would ultimately live in a service anyway, and it's just a matter of if we expose an attribute on the map directive to manipulate it.

@tomwayson
Copy link
Member

@Kollibri and @jwasil see 2b39c54 for latest API. I've removed the directive and replaced w/ a service function that can be called as seen in the custom basemap test page.

@Kollibri
Copy link
Contributor Author

@tomwayson awesome! I've updated my local copy to reflect these changes. Makes much more sense. Thank you!

@tomwayson
Copy link
Member

@Kollibri great!

BTW - there may be some fortunate timing still at work in that example page (requiring basemaps module always returns before requiring map modules). That should become apparent as I continue my refactor. If so, I will also add an attribute to the map directive that calls addCustomBasemap(), but I don't anticipate that API changing so you should feel free to use it as is. Also, you can get around any timing issues (if any) by calling that in a route's resolve.

@jwasilgeo
Copy link
Contributor

✋-5, y'all

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants