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

Image editing: batch editing in cropper component #23284

Merged
merged 9 commits into from
Jun 21, 2020
Merged

Conversation

ellatrix
Copy link
Member

Description

The difference with #22959 is that this uses the cropper for all edits. To do this, I've removed flipping for now, which is probably a lesser used feature anyway. I copied the PHP from the other PR. The purpose of this PR is to unblock iterations and possibly remove the experimental flag for the next GB release.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Jun 18, 2020

Size Change: -576 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-library/editor-rtl.css 7.78 kB -92 B (1%)
build/block-library/editor.css 7.78 kB -91 B (1%)
build/block-library/index.js 129 kB -393 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.26 kB 0 B
build/block-directory/style-rtl.css 937 B 0 B
build/block-directory/style.css 937 B 0 B
build/block-editor/index.js 107 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/style-rtl.css 8 kB 0 B
build/block-library/style.css 8 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 196 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.6 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.5 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.43 kB 0 B
build/edit-widgets/style.css 2.43 kB 0 B
build/editor/editor-styles-rtl.css 468 B 0 B
build/editor/editor-styles.css 469 B 0 B
build/editor/index.js 44.7 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 663 B 0 B
build/nux/style.css 660 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill added [Block] Image Affects the Image Block [Type] Performance Related to performance efforts labels Jun 18, 2020
@TimothyBJacobs
Copy link
Member

Would we then be able to drop the specific edit endpoints and just use /apply?

@ajlende
Copy link
Contributor

ajlende commented Jun 18, 2020

When moving all edits within react-easy-crop there isn't an option to rotate without cropping to one of the preset aspect ratios. I think that you're right about the flip probably being a lesser used feature, but I think it's important to be able to rotate without cropping.

@TimothyBJacobs TimothyBJacobs added the REST API Interaction Related to REST API label Jun 18, 2020
@ellatrix
Copy link
Member Author

@ajlende I’ll add it tomorrow morning

@ellatrix
Copy link
Member Author

@TimothyBJacobs yes let’s drop the other endpoints and also keep the whole api experimental

@mtias
Copy link
Member

mtias commented Jun 19, 2020

I like this path, it allows us to simplify the flow a bit (going through the Apply action). I also think it's good to mark the endpoints experimental, it seems like a model we should be willing to try in general as it'll allow us to gather feedback from developers widely during a cycle before committing to a final shape.

@mtias
Copy link
Member

mtias commented Jun 19, 2020

A few notes:

  • This button still shows up with the experiment disabled.

image

image

  • The aspect ratio is not working very well for me (gets reverted after "apply").
  • Can we improve the increments of the "zoom"? The jumps between steps are too abrupt and the number is not very intuitive (percentage would be better, I think).
  • It should likely use @ItsJonQ input number control set to percentage (so it shows the percentage sign).

Design:

image

  • I'd still use the "crop" icon instead of "edit".
  • We should move "Replace" to the right, after "link" and after "crop". Seems awkward there.
  • Cancel and Apply should be grouped in the toolbar, and in the right order.
  • We should hide all the other toolbar options when entering into edit mode (alignment, ellipsis menu)
  • The zoom component should be rendered in the toolbar.

@ellatrix
Copy link
Member Author

@mtias Yes, I see some issues too when applying the image. Looks like something isn't processed on the server. I'll have a look.

Agree on Zoom. Let's try to move it to the toolbar in a separate PR.

@ellatrix
Copy link
Member Author

This button still shows up with the experiment disabled.

Fixed

The aspect ratio is not working very well for me (gets reverted after "apply").

Not sure if I have the same problem, but I do have problems with square aspect ratio, which I'm looking into now. Seems like sometimes cropping on the server is not correct, probably instructions being processed wrong.

Can we improve the increments of the "zoom"? The jumps between steps are too abrupt and the number is not very intuitive (percentage would be better, I think).
It should likely use @ItsJonQ input number control set to percentage (so it shows the percentage sign).

Let's do that when we work on the other zoom things.

I'd still use the "crop" icon instead of "edit".

Fixed

We should move "Replace" to the right, after "link" and after "crop". Seems awkward there.

Fixed

Cancel and Apply should be grouped in the toolbar, and in the right order.

Fixed

We should hide all the other toolbar options when entering into edit mode (alignment, ellipsis menu)

Partly fixed. The general block buttons will have to be hidden in a special way as they're out of the block's control. Removed the other buttons though.

The zoom component should be rendered in the toolbar.

Separate PR

@ellatrix
Copy link
Member Author

ellatrix commented Jun 19, 2020

There's also issues with cropping in current master. When I crop to a different aspect ratio, it turns out much longer. Not sure why. There's some issues on the server side.

master:

image-miscrop

@TimothyBJacobs
Copy link
Member

So is this creating a new record in the media library?

@ellatrix
Copy link
Member Author

@TimothyBJacobs Once you apply the edits, yes

@TimothyBJacobs
Copy link
Member

In that case, can we instead return the results of a rest_do_request( '/wp/v2/media/{id}' ) and add a Location header with a 201 status code instead of the array? Something like.

$path = '/wp/v2/media/' . $attachment_id;
$response = rest_do_request( $path );

if ( ! $response->is_error() ) {
    $response->set_status( 201 );
    $response->header( 'Location', rest_url( $path ) ); 
}

@ellatrix
Copy link
Member Author

@TimothyBJacobs Let's review the REST API endpoint separately. For this PR, I'd like to focus on doing the edits in a single request on apply and some user facing polish. I'm going to merge this PR since we'd like to include it for the Gutenberg release.

@ellatrix ellatrix merged commit 5886225 into master Jun 21, 2020
@ellatrix ellatrix deleted the try/image-batch branch June 21, 2020 21:16
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 21, 2020
@TimothyBJacobs
Copy link
Member

Sure thing, is there an open issue for the PHP side of things? Does this invalidate #22959? I'm having trouble tracking the related issues.

I'm going to merge this PR since we'd like to include it for the Gutenberg release.

The endpoint's return value would impact the client code. So if this Gutenberg release is the same as the 5.5 release, it'll need to be addressed before then as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block REST API Interaction Related to REST API [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants