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

Add composer.json, so we can use wpackagist to include the plugin #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dword-design
Copy link
Contributor

No description provided.

@michaelzangl
Copy link
Owner

Nice.

I have to check the license though, because if I add it somewhere for inclusion I'd rater use MIT or something similar. Have to check if I used any code that is not mine.

@michaelzangl
Copy link
Owner

Just checked. The only thing I use is guzzle (just because I am no real fan of the HTTP options wordpress provides and I needed something that was light so that I don't have to initialize all of WP for a simple image request). Currently the dist files are just dropped to this repository, in the long run it might be better to pull them using compozer.

So if you are fine with it (since you are now the only other contributor to this repo), I would like to set the license to MIT. This makes distributing it with themes / modifying it for non-WP sites easier.

@michaelzangl michaelzangl added the enhancement New feature or request label May 24, 2018
@dword-design
Copy link
Contributor Author

Sure no problem with the license. It was primarily for the composer stuff, so feel free to change it.

Copy link
Owner

@michaelzangl michaelzangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dword-design
I'm currently reworking the deployment of this plugin (and some others I made) to upload the current version to the official WP plugin directory.

While doing this, I could also add a package.json to all releases. But I don't really understand the use of this since wpackagist is adding them as well. Where do you expect this file to be added?

.empty()
.append(jQuery('<div class="video-wrapped-play">').html(jQuery(this).attr('data-embed-play')))
.append($('<div class="video-wrapped-play">').html($(this).attr('data-embed-play')))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that Wordpress is using the $ shortcut for jQuery, but I'd prefer to keep the long version for compatibility.

@dword-design
Copy link
Contributor Author

Regarding the composer.json there is still the dependency of Guzzle. If you mix composer projects with pasted libraries, this leads to double definitions and incompatibilities. I manage all my PHP projects with composer. Despite that you‘re right – wpackagist creates a composer.json for you.

Regarding jQuery, I think I switched to $ for convenience but it doesn‘t really change much. As you like.

@michaelzangl
Copy link
Owner

I don't think this will be much of a problem since I am only using this composer library for the PHP script that proxies the image from e.g. youtube. For performance reasons, I don't want to load a full Wordpress there and that was the easies way to include an universal HTTP client in this script. It should not interfere with any other WP plugin since it is never loaded while WP is loaded.

@tyrann0us
Copy link

@michaelzangl, any progress here? I'd love to see the composer.json included in this package.
If you want, I can submit a cleaner PR that only includes composer.json, no other changes.

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

Successfully merging this pull request may close these issues.

3 participants