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

Wp Favs integration #690

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Wp Favs integration #690

wants to merge 5 commits into from

Conversation

timersys
Copy link

Hi Guys as commented on #687 here is the pull request with a simple integration of Wp Favs.
I just added one new function to grab the plugins from the API and updated the example.php with a working token example.

Thanks!

ps: Why you use themes as default screen for installing plugins? Just curious

@jrfnl
Copy link
Contributor

jrfnl commented Aug 29, 2017

Hi @timersys, I haven't looked at the code yet, but I like the idea of this.
However, users can also have a favorites list on wordpress.org. So I'm wondering if this PR would be accepted, if the favorites list on WP.org shouldn't also be supported.
Supporting an external service, but not a WP native implementation feels arbitrary.

Also, as this is a new feature, I think it should go into a minor release - in contrast to a patch release, like the 2.6.2 version we're currently preparing for -, so I'm tagging this with 2.7.0. /cc @GaryJones

As a side-note: you may want to have a look at the Travis output. The codestyle errors will need to be fixed before this PR can be considered: https://travis-ci.org/TGMPA/TGM-Plugin-Activation/jobs/268774079#L685-L760

@jrfnl jrfnl added this to the 2.7.0 milestone Aug 29, 2017
@timersys
Copy link
Author

Hey @jrfnl , I could add the native wp.org favorites support. I already got that bit of code so no problem with that.
I will review everything and update accordingly.

@timersys
Copy link
Author

So I added the wp favorites feature. Take in consideration that I added these 2 features as separate functions and didn't integrate them in the main class as I wasn't sure you wanted to go that route.

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.

2 participants