-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 Rich image editing capabilities to Gutenberg #21024
Conversation
7895448
to
c440834
Compare
c440834
to
e15c736
Compare
e15c736
to
c35716d
Compare
I see the feature added by default in the Gutenberg plugin, or am I missing something?
That's good if the general direction is agreed upon. The problem is that there's no easy, small PR to go from editing on the server to editing in the browser. The PR will need to replace it almost entirely.
I'm not talking about previews in the browser, but rather editing it in JS and sending the edited image to the server on save or autosave or some other trigger. There's no for the server to edit the images. I any case, thanks for working on this! |
const { url } = attributes; | ||
const isEditing = ! isCrop && isSelected && url; | ||
|
||
if ( ! isSelected ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is causing the block to rerender when it is selected, which is not a great user experience esp. with an image that needs to load.
<Fragment> | ||
{ noticeUI } | ||
|
||
<div className={ classes }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the div wrapper? I don't see why image editing tools should affect the structure of a block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The in-progress spinner needs a container to be centered properly on top of the image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rethink the spinner so we don't have to have the div. Can be a fading in and out image which I believe we use to indicate transience elsewhere. Happy to work with you on this 👍
Experimental, as in Gutenberg phase 2 (#21024 (comment)) as to not ship with core. @talldan also mentioned adding it to the experiments page, which I can go ahead and switch to instead since this is a bigger feature. EDIT: PR #22870 is now open
Which benefits do you think we'll get from editing in the browser instead? Using |
'permission_callback' => array( $this, 'permission_callback' ), | ||
'args' => array( | ||
'direction' => array( | ||
'type' => 'enum', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum
isn't a type, it should instead be string
.
'permission_callback' => array( $this, 'permission_callback' ), | ||
'args' => array( | ||
'cropX' => array( | ||
'type' => 'float', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
float
is not a json schema type, it should be number
.
* @param integer $height Height in pixels for the crop. | ||
*/ | ||
public function __construct( $crop_x, $crop_y, $width, $height ) { | ||
$this->crop_x = floatval( $crop_x ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using floatval
is the types are documented as integer
? Which is correct?
* @return array Updated metadata. | ||
*/ | ||
public function apply_to_meta( $meta ) { | ||
$meta['cropX'] = $this->crop_x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these based off an existing pattern that means these need to be camelCase
? Core uses snake_case
in PHP land.
Instead of using The REST API also uses The REST API meets every Thursday at 18:00 UTC in slack at #core-restapi. If you aren't seeing feedback from the REST API team, drop a link to the PR during our open floor. |
One note, this PR doesn't add a new block but some of the functionality added here makes the impression that a new experimental block is being registered. Given that this functionality is added using a filter, it should be moved to a different package, |
If we can address the outstanding issues promptly, I don't think we need to add it to experiments. The discussion on how much to do on the client vs server is a bit nuanced. It makes sense to me to do all manipulation client side and then commit the updates as a new image on "save". Stacking new images on the server every time would be taxing on storage space and memory usage for shared hosts. |
Related ticket: https://core.trac.wordpress.org/ticket/49096 |
Ok, even if you could do previewing in JS or even CSS, why do a round trip with an image to the server at all then, if it can be done on the client side? That's double work. Even more than double because the image is being transferred over the network for each edit. And regardless of that, the preview might not be exactly the same. The algorithm on the server might be different. Things like filters suddenly become more difficult to do, while if we just have to handle the client, it’s fairly easy. |
Doing the edits on the server was under the assumption that #22579 would be implemented so the request only happens once on save, so you wouldn't be downloading an image per edit. You'd POST the instructions for the edits and only download the final image. When editing in JS, since you send the image to the server, aren't you going to want to replace the image in the editor with the URL of the saved image to make sure the save worked properly for the preview? That was my assumption, and that way you're both uploading and downloading the image. The algorithms for crop, rotate, and flip are very straightforward, so I don't think there would be much of a chance for the algorithms to be different for them. I was thinking that filters would continue to be done in CSS like they are in Automattic/block-experiments, but I forgot that CSS filters aren't supported in IE11. It could still be done with SVG filters which are supported across browsers—I started a demo for a duotone filter a while ago in Automattic/block-experiments#88. Still, it might be nice to save the filtered image as well. In that case filters would have more opportunity for differences since you'd have to replicate the SVG filter on the server. |
I think filters are not done in CSS for the same reasons rotate and flip are not done in CSS on the front end? The image itself should be adjusted because the content might not include the CSS or might be distributed by email, RSS, social networks, etc. Additionally, if you download the image from the website, you'll get the original image. Generally people are not expecting anyone to have access to the original images. With the client already doing the edits, I don't see why we should add the complexity of doing the same thing on the server, if we can just upload the image on save/publish. I could also imagine Gutenberg being used with another API of a CMS that doesn't support image editing. Editing in JS means using the common API of creating/uploading an image, while editing on the server means additional endpoints are required. |
It would be ideal if that works :) Editing images in the browser (also before uploading them) is the best option. Been looking at that for quite some time, but seems "we're not there yet", the browsers still aren't particularly good image editors. Known problems with HTML canvas based editors (afaik):
Would be great if some js based image editors can be evaluated again, there seem to be few that are more popular. |
I think it might be worth separating the conversation on filters. These were experimental, and might not even be a fit for the core project, regardless of previous implementation. If they are (even just making a photo black and white and adjusting contrast would be delightful), it feels a separate conversation. One which I look forward to having, by the way :) |
Regarding EXIF data on JPG images, the edited image would have access to the original image data on the server, so we could adjust the function to get the EXIF data to get it from the original, if so desired. Not sure if that would be considered an ugly fix. Alternatively we can try to preserve it in JS, I would need to research it. Regarding the quality, are there any articles about this? I don't really see a problem at first sight. Yes, that's really annoying for phones and old/slow computers... what is the limit on a server? |
This is a bug with re-rendering, that will be fixed. The caption field should not even be visible in the setup state. Thanks for the report, @RickorDD |
About editing images in JavaScript: There are tons of features that depend on native WordPress image editor that are completely sidestepped by implementing image editing in JavaScript using canvas, the first thing that comes to mind is jpeg_quality filter that allows users and plugins to alter the JPEG quality. WordPress also allows hooking into the image processing so plugins like image optimizers can process the images. It's also possible to completely replace the WP_Image_Editor with another class to customize the image processing, for example using an image processing library like vips instead of GD or ImageMagick. If a canvas is used for image editing instead of predictable quality and scaling settings, the results will vary depending on which browser or OS was used when editing the image. Replacing native server-side WordPress functionality with JavaScript may seem trivial at first but it has a lot of consequences that need to be taken into account. |
Is there a PR somewhere for addressing the REST API related issues? As well as an overall tracking ticket for image editing? The REST API team is concerned about the amount of server side REST API code that is needed with this current approach so late in the cycle if we want the feature to land in 5.5. There is already a large amount of complex REST API integrations that are slated for 5.5. The only way I can see this landing in 5.5 is by doing editing in JS and adding support for https://core.trac.wordpress.org/ticket/49096. |
@TimothyBJacobs I didn't get to your edits because I was going to do a batch editing endpoint instead of all the individual ones in #22959. It's taken longer than expected, so I opened up #23037 for those edits now. |
can we add more aspect ratios to the crop feature? and how do we go about doing this? |
I am adding in this associated issue |
Description
Copied from #20637
Fixes #13748
Props to @johngodley and @jasmussen for the heavy lifting.
How has this been tested?
Screenshots
New toolbar items
Zoom crop mode
Types of changes
New features: Adds crop, flip, rotate capabilities to images.
Checklist: