Skip to content
This repository was archived by the owner on Apr 9, 2018. It is now read-only.

It's just angular-esri-loader #16

Merged
merged 4 commits into from
May 23, 2017
Merged

It's just angular-esri-loader #16

merged 4 commits into from
May 23, 2017

Conversation

tomwayson
Copy link
Owner

@tomwayson tomwayson commented May 15, 2017

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:

If 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?

@tomwayson tomwayson changed the title It's _just_ angular-esri-loader It's just angular-esri-loader May 15, 2017
@jwasilgeo
Copy link
Collaborator

@tomwayson absolutely!

@TheKeithStewart
Copy link
Collaborator

@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

@TheKeithStewart
Copy link
Collaborator

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.

@tomwayson
Copy link
Owner Author

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.

@andygup
Copy link

andygup commented May 15, 2017

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.

@TheKeithStewart
Copy link
Collaborator

I'm also good with the changes proposed.

@TheKeithStewart
Copy link
Collaborator

@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.

@tomwayson
Copy link
Owner Author

@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.

@tomwayson
Copy link
Owner Author

tomwayson commented May 23, 2017

@TheKeithStewart

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.

@tomwayson tomwayson merged commit 268b586 into master May 23, 2017
@tomwayson tomwayson deleted the angular-esri-loader branch May 23, 2017 04:04
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.

The naming problem
4 participants