closes #600

Implementation notes

• We need to know the original image width and height before allowing to resize the image, this is done by loading the image before showing it and limiting its width/height to the canvas size (see ImageSize component)

Testing instructions

• Try inserting and resizing an image

Questions

Should we resize the container (image + caption) or just the image?

Holy guacamole you're so fast it's hard to keep up!

This works really really well.

I can't decide whether we should change the block behavior when the image has no alignment. But it does bring some challenges with it, when it comes to floats and centering:

It seems right now we hard-code a width when we float. Can we undo that?

I have a bad feeling we might need to revisit the full-wide hack we've been using, or at least rethink how images float in such a world. Which is on me, but let's see what we can do with the floated block width for now...

Thanks Riad!

There appear to be some issues with the demo content in this branch:

@jasmussen Fixed the demo content bug but for the left/right width it's way harder. The width is fixed using the $float-margin mixin but It's hard to override when the width is dynamic. I think you're right, we need to revisit the full-wide hack we've been using to make this work.

All the help here would be appreciated (I don't have all the context about this hack) and maybe we could still ship this and resolve the left/right fixed width in another PR by changing the hack.

Pushed a change to account for resized images differently on floated instances (also centering on wider alignments).

The only slightly annoying thing is that, when the toolbar is wider than the image, it forces the width to be bigger:

When you unselect the image it goes back to the right size.

@jasmussen also maybe "wide" and "full-width" could clear the resized values.

Vast improvements in a short amount of time, impressive. My hopes that we don't need to rewrite the full width math are restored.

Got some niggles still, though:

Disabled the resizing for wide/full alignments but there're still some glitches for the left/right alignments.

Not sure I understand @mtias's fix but I found that always applying "data-resized" to the wrapper makes the component behave like I wanted.

@youknowriad if the image is not resized we'd want the 370px max-width to apply when floated.

@mtias I'm not sure this is true, floating means "only floating" for me and frontend styling doesn't include this max-width.

Yes, it hasn't been applied to front-end, and we may want to revise that, but that was the intention of having the max-width, because if the image is larger than the text column, it doesn't look floated at all.

@mtias What about setting the 370px width to the image attributes when we switch to the left/right alignment (if no size set before) and always set data-resized. This will fix both issues at once.

Let's try it.

Ok updated, It works well IMO but there's one caveat: I had to add a 'resized' attribute to the image to differentiate between explicitely resized image and a size set automatically when applying float alignments. This boolean is used when moving to "center" alignment: if we explicitly resized the image, we keep the size unless we reset it.

There's some small remaining issue where we set a right value in the width/height attributes but the component is not rerendered accordingly unless we refresh.

There are a couple of issues still. But if we believe these are issues we can address, then it's worth getting this in as a first pass, and then iterating. If, on the other hand, any of these sound like we can't address them easily, might be worth rethinking.

1. The resize handle should only be visible when the block is selected
2. If you resize an image reeeeally tiny, the handle doesn't follow all the way down. And when you later size up, the handle is out of sync.
3. It feels like we should do the same as the current editor does with regards to image widths, when floated. Applying a max width seems arbitrary. Maybe you WANT a giant image that's left aligned.
4. When you size an image bigger up, you can invoke multi select if you move the cursor downwards. This is easiest to test while resizing a thin image, like the landscape image in the demo content.
5. There seems to have been a regression with images in the post content that are wide and full wide. Then handles there are gone, as they should be, but if you set one of those wide images to "float left" or "float right" instead, you have to first resize them down from their giant wide size first. And so, if you are sizing down an image in the demo content from fullwide, we should probably set the main column width first.

2 and 4 seem like they can be fixed both, by applying mininum and maximum dimensions.

Codecov Report

Merging #2213 into master will decrease coverage by 0.06%.
The diff coverage is 6.52%.

@@            Coverage Diff             @@
##           master    #2213      +/-   ##
==========================================
- Coverage   25.35%   25.28%   -0.07%     
==========================================
  Files         151      152       +1     
  Lines        4701     4797      +96     
  Branches      792      816      +24     
==========================================
+ Hits         1192     1213      +21     
- Misses       2967     3026      +59     
- Partials      542      558      +16

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4dc3e0...1c30f9a. Read the comment docs.

@jasmussen Updated with another approach that seems simpler and more robust. Mind taking a look?

Love this feature, thank you for working on it.

Can't feel a material difference in the new approach, though.

I think we should consider looking at some min/max boundaries, and considering multi select triggering (I suspect multi select might also put cogs in the wheels of drag and drop down the road):

I think we should consider looking at some min/max boundaries,

This is not hard to achieve but I don't think we should do it, themes have different max-width/height than the editor.

Multiselection

Not an easy problem to solve, I thought of a flag set to true in the state while resizing but it's not easily achievable because the blocks are technically not aware of the store (state) container (unless we continue our work of merging the blocks/editor modules). An option would be to provide startResizing, stopResizing as a prop to the edit function but obviously, this is not ideal. I suspect we'll have this multi-selection issue whenever a block uses drag and drop somehow (cc @iseulde @aduth )

This is not hard to achieve but I don't think we should do it, themes have different max-width/height than the editor.

Wanted to clarify that I meant meant boundaries to resizing. Although it looks like there's no minimum size TinyMCE can resize images to, there's a max size. You can't size the image larger than the viewport.

themes have different max-width/height than the editor.

This is something we'd like to address with the theme notifying the width of the main column to us. Widths that go beyond that should be done with the "wide" alignment, I think.

Per slack discussion: I set the minimum width/height to 20px and the max-width to the image width.

Thank you for working on this.

I'd love for the minimum dimensions to be 10px rather than 20px. But more importantly than those dimensions, I think, is that the handle detaches if you scale it smaller than what appears to be 20px height, and when you then scale up again, the handle sits in the air:

Scaling up works 200% better now! It's almost perfect. But we may need scaling more scaling handles, or we're stuck when you make the image too big:

It'd be nice (though not necessary for this PR) that the handles are hidden until you select the block.

For reference, here's what TinyMCE does:

Note how if you scale larger than the viewport, the sizing is discarded.

Note how if you scale larger than the viewport, the sizing is discarded.

TinyMCE limits scaling to the editor width and not the browser viewport. So, this seems more logic to me. For us, this might mean limit the scaling to the editor's content area size (which is 700px IIRC) but again, this means the user can't scale more than this even if it's theme allows it.

Also, the difference with TinyMCE is that Tinymce's size is flexible (it grows if you resize the window) which is not the case for our editor's content area.

I'm looking at the issues above

the handle sits in the air:

This is fixed

It'd be nice (though not necessary for this PR) that the handles are hidden until you select the block.

This is fixed

But we may need more scaling handles

This can't be done with the current resizing library I'm using :(. Two options: find another library or write our own. The second one could be difficult to do. I'm looking at alternatives.

TinyMCE limits scaling to the editor width and not the browser viewport. So, this seems more logic to me. For us, this might mean limit the scaling to the editor's content area size (which is 700px IIRC) but again, this means the user can't scale more than this even if it's theme allows it.

To be clear, it's fine to improvements in this area in separate non-milestoned PRs.

This can't be done with the current resizing library I'm using :(. Two options: find another library or write our own. The second one could be difficult to do. I'm looking at alternatives.

Bummer. Hmm. Can we choose which corner the handle sits on?

The big problem right now is that it's very easy to scale the image so large that the handle comes out of view, and then you can't scale it down. Perhaps we do need to limit the size to the editor width, so as to prevent this from happening.

Not saying it has to be perfect, looking for quick fixes here.

@jasmussen Found a better library, let me know what do you think?

🎉🎉🎉🎉

Oh my science this latest one is AMAZING! It feels completely solid! I think this can go in virtually as is!

A future improvement could be to improve this:

and this:

That is it'd be nice if the caption field was no larger than the image itself, and when the image is selected it seems like we might want to remove the 370px altogether on floats (which lets us remove the !important that Matías added).

The thing is, the max size on floated images is something we added because we didn't have a good resizing/insert thumbnail play. Now that we have good resizing, I don't think we need it.

A future improvement could be to improve this:

Do you mean we should resize the whole "figure" node (image + caption) instead of resizing the image (current behaviour)? What do you suggest to fix those?

Do you mean we should resize the whole "figure" node (image + caption) instead of resizing the image (current behaviour)? What do you suggest to fix those?

I'm not sure what the best way to improve this is. The idea would be that the caption field is no wider than the image. But let's explore this separately, and it doesn't have to be in the milestones.

For us, this might mean limit the scaling to the editor's content area size (which is 700px IIRC) but again, this means the user can't scale more than this even if it's theme allows it.

I think this is fine. If themes were to modify this, it would no longer be 700px in the editor.

Not sure I understand, do you think this is ok to ship or is there anything else I should look at?

I meant not going beyond the width set on blocks that don't have wide/full alignment.

👍 Understood and updated to constrain the resizing within the canvas. The value is hard-coded right now but we could easily make it overridable later.

Love it, I think it's good to go!

We do definitely want ro revisit the max dimensions, but let's do that as part of a larger editor width discussion. I know there's a ticket for this separetely too. As it stands, this PR feels solid 👌