search

ng2-ui / auto-complete

Angular Auto Complete component and directive
https://ng2-ui.github.io/dist/#/auto-complete
MIT License
279 stars 123 forks source link

Update to angular 4.3.0+ #266

Closed kennyrulez closed 7 years ago

kennyrulez commented 7 years ago

In order to make it compatible with the new httpclient.

Remove from demo app the reference to Angular Material, because is using a deprecated version.

allenhwkim commented 7 years ago

Please make PR without build.

allenhwkim commented 7 years ago

This may conflict to #265. I will accept it after #265 is merged.

almothafar commented 7 years ago

It is no need to merge this also it will conflict with #265 in that pull request I used the latest material not removed it. I reviewed your pull request I think you need to keep material just use latest, and do HttpClient back, and pull request without dist folder.

kennyrulez commented 7 years ago

I reviewed @almothafar PR #265 and I think his version is better than mine. He added some interesting features which must be in the next release. As of solely purpose of demonstration, I don't think Angular Material is crucial for the functionality of the plugin itself and I wont spend time on it, but more is better so welcome to examples with it. In my general idea, all modules must be agnostic of any design, letting the final user to decide how to implement them.

I understand that keeping the old http module is crucial for compatibility but usage of the new HttpClientModule is way better than the previous one: is speedier and the revamped availability of interceptor could make a great use for future release of this plugin. The caching one is a must have to reduce the amount of requests to the server: I'm implementing it in my local development and will push to github if functional sometime later these days. @allenhwkim maybe you can create a different branch for version 2 to 4 and a branch for 4.3+?

almothafar commented 7 years ago

@kennyrulez the demo is minor anyway and does not affect the addon itself, all these things are in dev and for a demo, and yes it is better to use HttpClientModule I really saw this after your pull request and searched about it and really interesting.

I'm not doing so much PRs so totally forgot to do branch, anyway, the real start for Angular is version 4, it is called just Angular not Angular2 after version 4, so I think most people already went to Angular v4 instead of Angular2, so it is Ok if someone wants to stay on Angular2, it does not make sense to use the newest version of this plugin, while he already still using old version of Angular itself.

kennyrulez commented 7 years ago

I somewhat find confusing the choice of naming for Angular from Google. If anyone search on google, stackoverflow or github itself, most of the project named or tagged with Angular refer to projects made for angular 1.x. So following the semantic versioning imposed by Big G is crucial. I personally love the "ngx-" moniker for angular2+ modules purposely to avoid this confusion and let dependencies (and a good readme.md) define which version a developer must use or adhere in order to use the module.

Anyway, you guys have made a really good work and I look forward to see your PR generally available in NPM soon.

I also hope the owner @allenhwkim will branch this project with a stable, feature complete version for 2 and < 4.3.0, and pumping the gas on master branch with 4.3.x+ compatibility.

allenhwkim commented 7 years ago

@kennyrulez Can I close this PR, and you can make another PR later? I have an issue with #265 at the moment although it is merged. To make things simple, I would prefer a new PR if you want to make it after #265 is fully merged(with bug fix) and released.

kennyrulez commented 7 years ago

Go and close it. I'll send a new PR as soon as I've implemented some other things. Thanks