angular-socialshare icon indicating copy to clipboard operation
angular-socialshare copied to clipboard

Refactor code into separate providers

Open hermansje opened this issue 9 years ago • 4 comments

After showing a modular rewrite, this is the pull request. Each social platform has its own provider so only the necessary providers have to be included. The service usage and the configuration of the defaults differ from the current implementation.

hermansje avatar Jun 19 '16 18:06 hermansje

Hi @hermansje this work looks stunning!

I have just some requirements, to make it happen. Since there are major changes to the code, let me know if you can make these edits please:

1 - Service() instead of Factory() (this is more an angular 2 request) 2 - Unique configProvider for all the providers (so that i can call once only socialshareConfProvider.configure({....all the providers in here})) 3 - Finally adjust README.md according to the new structure/usage

One more thing is: we should, anyway, serve angular-socialshare.js with all the providers already included, then serve all the separated provider files plus angular-socialshare.lite.js for example. So that (by default) including angular-socialshare.js, you'll have all the providers ready to be used, and switching to angular-socialshare.lite.js will mean you'll have to include manually each of the provider files you want to use. This can be achieved just by some new task for gulp, so if you don't want to waste time i can easily check this later, no problems.

The final dist/ folder should look like this:

dist/
   angular.socialshare.min.js (everything inside, no need for extra files)
   angular-socialshare.lite.min.js (need each provider file)
   angular-socialshare.facebook.min.js
   angular-socialshare.twitter.min.js
   angular-socialshare.vk.min.js
  //and so on..

Last thing: please send PR on a new branch (https://github.com/720kb/angular-socialshare/tree/modular) if you can, just because changes are major for the master branch atm 👍

Is it ok for you?

Thanks.

45kb avatar Jun 21 '16 09:06 45kb

:+1:

alonronin avatar Jul 19 '16 14:07 alonronin

is it going to be merged ?

YanivPlaybuzz avatar Aug 30 '16 11:08 YanivPlaybuzz

@YanivPlaybuzz Unfortunately not. There still missing important points i highlighted in my previous comment

45kb avatar Aug 31 '16 06:08 45kb