Skip to content
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

Upgrade to Angular 2.0.0 RC5 + encapsulte into NgModule #48

Merged
merged 4 commits into from
Aug 23, 2016

Conversation

bernhard-niedermayer
Copy link
Contributor

@bernhard-niedermayer bernhard-niedermayer commented Aug 19, 2016

solves #47

Bernhard Niedermayer added 2 commits August 19, 2016 14:56
The library has been updated to Angular 2.0.0-rc5
Toaster has been encapsulated in an ngModule
Declarations that became obsolete have been removed
Test setup has been adapted according to Angular 2.0.0-rc5
Remove unused variables

Closes Stabzs#47
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.06%) to 98.942% when pulling 4857af9 on bernhard-niedermayer:master into 87ca86e on Stabzs:master.

@lacolaco
Copy link

lacolaco commented Aug 22, 2016

please move @angular/* dependencies into peerDependencies or devDependencies because I want to use this package with nightly-builds packages like angular/core-builds.

@Stabzs
Copy link
Owner

Stabzs commented Aug 22, 2016

@bernhard-niedermayer Thank you so much for all of your hard work on this! It was a ton of cleanup in the demo projects and much-needed optimizations and it is greatly appreciated!

A few fixes please, before this can be merged.

exports.TostModule in angular2-toaster-js is a typo and should be updated to exports.ToastModule please.

The test coverage dropped because you modified the callback on line 712. This incorrectly causes that path to no longer get tested. Please adjust test to return coverage back to 100%.

Once these two modifications are made, I'll be more than happy to merge this in.

@Stabzs
Copy link
Owner

Stabzs commented Aug 22, 2016

@laco0416 I'm having difficulty justifying moving the angular2 dependencies to peerDependencies just to support nightly builds. This will certainly cause a lot of confusion for a number of users when their installs proceed with just a warning but the version isn't present for a build.

Moving it to devDependency is flat out incorrect and a poor representation of the dependencies for the library.

@lacolaco
Copy link

@Stabzs does "users" mean contributors? almost users install explicitly angular2 and then install plugins. usually, peerDependencies doesn't cause confusion.

if some users (including me) want to use nightly-builds, some warning messages will appear. it's no problem because they want to go off the standard way.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.06%) to 98.942% when pulling 0e4fcb2 on bernhard-niedermayer:master into 87ca86e on Stabzs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 58ced25 on bernhard-niedermayer:master into 87ca86e on Stabzs:master.

@bernhard-niedermayer
Copy link
Contributor Author

I have just added 2 commits.
The first one to fix the typo in angular2-toaster-js,
the second one to fix the test.

@Stabzs
Copy link
Owner

Stabzs commented Aug 23, 2016

@laco0416 I meant in the context of consumers of the library.

However, you make a great point...this library is a plugin and peer dependencies fit plugins better since that way they can be dropped in more easily. I'll update the dependency type.

@Stabzs
Copy link
Owner

Stabzs commented Aug 23, 2016

@bernhard-niedermayer thank you for making the changes and thank you for your hard work on this!

@Stabzs Stabzs merged commit 1ab2577 into Stabzs:master Aug 23, 2016
@Stabzs
Copy link
Owner

Stabzs commented Aug 23, 2016

A new npm release will be created after I've updated README documentation to match NgModule-style inclusions.

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

Successfully merging this pull request may close these issues.

None yet

4 participants