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: Batch image edits #22959

Closed
wants to merge 16 commits into from
Closed

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Jun 5, 2020

Description

Adds the ability for edits to be previewed in the browser before saving the file on the backend.

TODO

How has this been tested?

TODO

Screenshots

TODO

Types of changes

Fixes #22580
Fixes #22579
Fixes #22566

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.

@ajlende ajlende self-assigned this Jun 5, 2020
@ZebulanStanphill ZebulanStanphill added the [Block] Image Affects the Image Block label Jun 6, 2020
@ajlende ajlende force-pushed the fix/batch-image-edits branch 2 times, most recently from cc80459 to 6de653b Compare June 8, 2020 23:37
@ellatrix
Copy link
Member

ellatrix commented Jun 9, 2020

@ajlende What's left here?

@ajlende
Copy link
Contributor Author

ajlende commented Jun 10, 2020

The quick list is:

  1. Clean up and organize the REST API code. It isn't very readable right now, old routes can probably be removed, etc.
  2. Decide on a naming schema for edited images now that all edits happen at once.
  3. Add crop CSS previews.
  4. Show rotated and flipped images in crop mode. react-easy-crop supports rotation, but not flipping, so I'm going to have to figure out a way around that.
  5. Fix container height when rotating. transform: rotate() doesn't update layout, so the image width isn't correct and it's still overlapping things above and below it right now.
  6. Adjust the toolbar and edit flow to match the mockups that @jasmussen made (Queue up image edits to only apply once all edits are complete #22579 (comment))

I'm afraid there isn't going to be a way around this being a big PR. Lots of the changes are interconnected.

@ellatrix
Copy link
Member

Decide on a naming schema for edited images now that all edits happen at once.

What are the options? If it's marked as experimental, we can still change it in the coming days.

Add crop CSS previews.

I think we'd like to move towards previewing in JS (using a blob url). This would be the closest structurally to replacing the src afterwards. I wouldn't get stuck too much on the previews in this PR.

@ellatrix
Copy link
Member

I'm wondering if we'll have to roll our own cropper/editor. This component could also be combined with the resizer that we currently have. If we reuse the same element, this would also avoid a re-render between view and edit mode (avoiding an image reload).

@spacedmonkey
Copy link
Member

Wouldn't it be better to use - https://core.trac.wordpress.org/ticket/50244 ?

@ajlende
Copy link
Contributor Author

ajlende commented Jun 11, 2020

@spacedmonkey Maybe it would be part of the answer? Right now the existing endpoints for crop/rotate/flip each generate a new image, and one of the main purposes of batching was to avoid that. Even if we passed a filename or overwrite flag, writing to disk for each image edit also seems like it might be quite slow. I'd also like to maintain the statelessness of the endpoints, so I wouldn't want to make the existing endpoints just queue up and not apply the edits. That's why I ended up with a new endpoint. I hadn't seen that trac ticket yet, so if you have other suggestions I'd be happy to hear 🙂

@ajlende ajlende force-pushed the fix/batch-image-edits branch from 6de653b to c2f8b5f Compare June 16, 2020 07:38
@ellatrix
Copy link
Member

If we're blocked at flipping the image, could we temporarily remove that option?

@paaljoachim
Copy link
Contributor

This seems more like a feature that could use time to mature in the Gutenberg plugin before being added to core. I would suggest holding off on 5.5 and instead give it more time and then add it to 5.6.

@ellatrix
Copy link
Member

I've merged #23284. I hope we can easily build further on that. :)

@ellatrix ellatrix closed this Jun 22, 2020
@ajlende ajlende mentioned this pull request Jun 22, 2020
6 tasks
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
Projects
None yet
6 participants