-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
@tomwayson absolutely! |
@tomwayson yes, the usage example and the gist should change to use the NgModule instead. I'll contribute to this PR shortly with the appropriate change. I'll go ahead and update the |
Sorry, I accidentally, temporarily closed this PR. Damn fat fingers typing on a mobile! I'll update https://github.com/tomwayson/esri-angular-cli-example/ once the new release is out. |
FYI - unless I hear vocal opposition I'm planning to just start angular-esri-loader at v1.1 (which is what this release would be if we were't changing the name since it adds the ngModule, but is not a breaking change). That just seems less confusing and less work than having the "angular2-esri-loader-v1.0.0" tag. |
No objections here. FWIW, the Angular versions are going to continue to iterate at a fairly fast rate. Given the approach you are thinking, having a more generic name may be beneficial in simplifying the logistics of managing this repo. |
I'm also good with the changes proposed. |
@tomwayson I pushed a commit that updates the usage example to load the NgModule. Let me know if there is anything else I can help with. |
@TheKeithStewart looks good, thanks! I haven't forgotten about this, just haven't had time to follow up yet. Will do so in the coming days. |
It occurred to me that b/c there have been problems using the NgModule in Angular 2 apps, that we should keep the old usage instructions for Angular 2. Let me know if that looks good. If it needs to be changed, we can do so outside of this PR. |
These are the code changes required to resolve #15
I've tested locally via
npm link
and this seems to work.In addition to these changes, after merging we'll need to:
rename v1.0.0 tag to angular2-esri-loader-v1.0.0If this looks good, I can take care of most of that, except as follows:
@TheKeithStewart
One thing I noticed, the gists and usage instructions all use the service directly (instead of via the NgModule) Do you think they should be changed? If so, would you mind adding a commit to this PR that updates the usage accordingly and I'll put that in the gists.
Also, once you update angular2-esri4-components to use angular-esri-loader, we'll need to
update esri-angular-cli-example
@jwasilgeo after I update my gist, would you mind updating yours?