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

Wp4.5 #54

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

Wp4.5 #54

wants to merge 30 commits into from

Conversation

RadGH
Copy link

@RadGH RadGH commented Apr 15, 2016

Here's a new collection of changes made to your plugin. This expands on a previous pull request I submitted earlier: #49

New changes since that pull request are:

  1. Converted quality dropdowns to numeric inputs, allowing sizes like 85% (This behavior may not be desired, if so then skip commit 07ce8aa)
  2. If you use Regenerate Thumbnails, a new option will re-crop an image after the thumbnails have been regenerated.
  3. Due to the above, I removed the warning that Regenerate Thumbnails was incompatible has been removed. This warning was added in the previous pull request.
  4. Fixed a few URLs which were not giving a full admin url. Probably not necessary unless you have a custom admin url set up.
  5. Fixed a few small code errors so my IDE (phpstorm) was happier.

Note: Some users on wordpress.org complained about problems with the dialog closing and not saving images. I have not experienced this issue with my build of the plugin. If you can replicate the problem in your own version, you might find this version works. I'm not sure if that's the case, I haven't tested the original in some time ;)

Radley Sustaire added 28 commits December 1, 2015 15:06
…ail on cropping images for future plugin support.
@noreabu
Copy link

noreabu commented Apr 19, 2016

Thanks for the effort. I merged it into my wip branch (consisting of #55 + #40 + #52 ) and it works fine.

But i wonder if this is necessary: 8b2aa44 because esc_attr() should imho be used to sanitize user-generated-content and i think we can trust the wordpress-function admin_url()

@RadGH
Copy link
Author

RadGH commented Apr 19, 2016

I would agree. I am unsure how to remove that commit from this pull request though. I am relatively new to collaborating with Git. What would you suggest I do? Or is that something you can fix yourself in a separate commit?

@noreabu
Copy link

noreabu commented Apr 20, 2016

I think the simplest solution would be to use git revert 8b2aa44 on your local wp4.5 branch. Then Push this branch again.

I am rather new to Github too, but some days ago i also made a pull request, then another commit to the same branch, and could easily submit that. If i make a commit, then it will be a seperate Pull Request from me.

@RadGH
Copy link
Author

RadGH commented Apr 22, 2016

Sorry about the delay. I think I figured it out and removed esc_attr. It looks like my new commits have been included automatically.

Thanks for the tip on reverting a specific commit!

@mcaskill
Copy link

@RadGH I've installed this plugin in a French locale, and I've spotted some JS errors caused by the translations for the "Crop Image" links that are injected by ManualImageCrop.php.

The french translations use an apostrophe which breaks the one used by JS to wrap the string.

The solution is to wrap those various values in esc_html_*() and esc_attr_*().

$mediaEditLink.after( '<a class="thickbox mic-link crop-image-ml crop-image"
rel="crop"
title="<?php esc_attr_e("Manual Image Crop","microp"); ?>"
href="' + ajaxurl + '?action=mic_editor_window&postId=' + match[1] + '"><?php
    esc_html_e('Crop Image','microp')
?></a>' );

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.

3 participants