Image Block: Adding image resizing handles #2213

youknowriad · 2017-08-04T09:03:47Z

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?

jasmussen · 2017-08-04T09:38:01Z

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!

jasmussen · 2017-08-04T09:43:03Z

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

youknowriad · 2017-08-04T12:18:06Z

@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.

aduth · 2017-08-04T13:26:57Z

.eslintrc.json

@@ -24,7 +24,8 @@
 		"wp": true,
 		"wpApiSettings": true,
 		"window": true,
-		"document": true
+		"document": true,
+		"Image": true


See also: #1008

It was intentional to remove all globals and encourage to access through window, i.e. window.Image. Could be easy to think one had assigned a local Image variable but in fact it's referencing the global (as in the case of event bug of #1007). I guess same could be said of document (window.document), but... ¯\_(ツ)_/¯

aduth · 2017-08-04T15:13:37Z

blocks/library/image/image-size.js

+		this.image.onload = () => {
+			const maxWidth = this.container.clientWidth;
+			const ratio = this.image.height / this.image.width;
+			const width = this.image.width < maxWidth ? this.image.width : maxWidth;


Math.min could come in handy here:

const width = Math.min( maxWidth, this.image.width );

aduth · 2017-08-04T15:14:32Z

blocks/library/image/image-size.js

+			const maxWidth = this.container.clientWidth;
+			const ratio = this.image.height / this.image.width;
+			const width = this.image.width < maxWidth ? this.image.width : maxWidth;
+			const height = this.image.width < maxWidth ? this.image.height : maxWidth * ratio;


Same note about Math.min, but also: Is maxWidth the correct value to test against? Or should maxHeight be a separate variable maxHeight = maxWidth * ratio ?

maxWidth is the correct value, we adapt the height only if the image don't fit in the container's width.

aduth · 2017-08-04T15:14:44Z

blocks/library/image/image-size.js

+			const ratio = this.image.height / this.image.width;
+			const width = this.image.width < maxWidth ? this.image.width : maxWidth;
+			const height = this.image.width < maxWidth ? this.image.height : maxWidth * ratio;
+			this.setState( { width, height } );


What if the component has unmounted by this point?

There's a componentWillUnmount call.

Edit: I have a typo there

Oh, missed that. Is changing the src while the request is in flight enough to prevent onload from being called?

And setting the src to a function?

I did some testing. It seems to prevent onload callback if changed while in-flight. Though I think we'd ought to set to an empty string, not sure what the expected behavior of a function is.

aduth · 2017-08-04T15:18:17Z

blocks/library/image/index.js

+								lockAspectRatio
+								onResize={ ( event, { size } ) => setAttributes( size ) }
+							>
+								<img src={ url } alt={ alt } onClick={ setFocus } />


Minor: We could create a single reference to this element to share between condition above and here:

diff --git a/blocks/library/image/index.js b/blocks/library/image/index.js index f6985bc8..766fb8aa 100644 --- a/blocks/library/image/index.js +++ b/blocks/library/image/index.js @@ -178,8 +178,9 @@ registerBlockType( 'core/image', { <figure key="image" className={ classes }> <ImageSize src={ url }> { ( originalWidth = width, originalHeight = height ) => { + const img = <img src={ url } alt={ alt } onClick={ setFocus } />; if ( ! originalHeight || ! originalWidth ) { - return <img src={ url } alt={ alt } onClick={ setFocus } />; + return img; } return ( <ResizableBox @@ -188,7 +189,7 @@ registerBlockType( 'core/image', { lockAspectRatio onResize={ ( event, { size } ) => setAttributes( size ) } > - <img src={ url } alt={ alt } onClick={ setFocus } /> + { img } </ResizableBox> ); } }

mtias · 2017-08-04T16:55:26Z

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.

mtias · 2017-08-04T16:56:41Z

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

jasmussen · 2017-08-04T16:57:38Z

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:

youknowriad · 2017-08-04T17:21:53Z

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

youknowriad · 2017-08-04T17:26:28Z

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.

mtias · 2017-08-04T17:30:35Z

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

youknowriad · 2017-08-04T17:34:27Z

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

mtias · 2017-08-04T17:36:15Z

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.

youknowriad · 2017-08-04T17:41:18Z

@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.

mtias · 2017-08-04T17:44:31Z

Let's try it.

youknowriad · 2017-08-04T18:09:30Z

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.

youknowriad · 2017-08-04T18:17:52Z

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.

jasmussen · 2017-08-04T18:27:16Z

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.

The resize handle should only be visible when the block is selected
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.
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.
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.
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 · 2017-08-04T18:28:45Z

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

Impacted Files	Coverage Δ
blocks/with-editor-settings/index.js	`60% <ø> (ø)`	⬆️
editor/index.js	`0% <0%> (ø)`	⬆️
blocks/library/image/image-size.js	`0% <0%> (ø)`
blocks/library/image/index.js	`12.24% <14.28%> (-2.47%)`	⬇️
editor/effects.js	`32.52% <0%> (+2.21%)`	⬆️

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.

youknowriad · 2017-08-07T09:11:13Z

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

youknowriad · 2017-08-09T09:35:13Z

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

jasmussen · 2017-08-09T09:50:47Z

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.

youknowriad · 2017-08-09T10:14:12Z

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

youknowriad · 2017-08-09T10:38:48Z

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.

jasmussen · 2017-08-09T10:54:54Z

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.

youknowriad · 2017-08-09T11:12:28Z

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

jasmussen · 2017-08-09T11:38:27Z

🎉🎉🎉🎉

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.

youknowriad · 2017-08-09T11:54:30Z

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?

jasmussen · 2017-08-09T12:02:02Z

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.

mtias · 2017-08-09T14:03:32Z

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.

… the alignment change

… of the inserted image

…ing handlers

youknowriad · 2017-08-09T15:06:08Z

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

mtias · 2017-08-09T16:40:21Z

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

youknowriad · 2017-08-09T17:06:52Z

👍 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.

jasmussen · 2017-08-09T17:12:43Z

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 👌

youknowriad added [Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media labels Aug 4, 2017

youknowriad self-assigned this Aug 4, 2017

youknowriad requested a review from jasmussen August 4, 2017 09:03

youknowriad force-pushed the add/image-resize-handles branch 2 times, most recently from 257b482 to 8a20ead Compare August 4, 2017 09:13

youknowriad force-pushed the add/image-resize-handles branch from 8a20ead to efe35da Compare August 4, 2017 12:14

aduth reviewed Aug 4, 2017

View reviewed changes

youknowriad force-pushed the add/image-resize-handles branch 2 times, most recently from cabd5d5 to d247219 Compare August 4, 2017 18:17

youknowriad force-pushed the add/image-resize-handles branch from d247219 to 2eda5e7 Compare August 7, 2017 09:09

youknowriad force-pushed the add/image-resize-handles branch from 2eda5e7 to 0a3dac3 Compare August 9, 2017 09:33

youknowriad force-pushed the add/image-resize-handles branch from 0a3dac3 to 3e0d3e7 Compare August 9, 2017 10:30

youknowriad and others added 9 commits August 9, 2017 15:55

Image Block: Adding image resizing handles

ff5c623

ImageBlock: Avoid the Image as a global and use window.Image

0793a71

Image Block: Fix image-size unmounting behaviour and small tweaks

d88fba1

Handle resized images without forcing a max-width when floated.

d5a408e

Image Block: disable resizing images if alignement is full or wide

829d986

Image Block: Change the width calculation mechanism to retrigger when…

73347fe

… the alignment change

Image Block: Resizing is limited from 20px to the actual width/height…

12a26e1

… of the inserted image

Image Block: Hide the resizing handle if the block is not selected

548e13f

Image Block: Changing the resizing library to allow adding more resiz…

06b88c3

…ing handlers

youknowriad force-pushed the add/image-resize-handles branch from dd646b7 to 06b88c3 Compare August 9, 2017 14:56

Image Block: Fix empty caption image

76798e8

Image Block: Constraint resizing within the editor canvas

1c30f9a

youknowriad merged commit 659b1bb into master Aug 9, 2017

youknowriad deleted the add/image-resize-handles branch August 9, 2017 17:20

Image Block: Adding image resizing handles #2213

Image Block: Adding image resizing handles #2213

Conversation

youknowriad commented Aug 4, 2017

jasmussen commented Aug 4, 2017

jasmussen commented Aug 4, 2017

youknowriad commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias commented Aug 4, 2017

mtias commented Aug 4, 2017

jasmussen commented Aug 4, 2017

youknowriad commented Aug 4, 2017

youknowriad commented Aug 4, 2017 • edited Loading

mtias commented Aug 4, 2017 • edited Loading

youknowriad commented Aug 4, 2017

mtias commented Aug 4, 2017

youknowriad commented Aug 4, 2017 • edited Loading

mtias commented Aug 4, 2017

youknowriad commented Aug 4, 2017

youknowriad commented Aug 4, 2017

jasmussen commented Aug 4, 2017

codecov bot commented Aug 4, 2017 • edited Loading

Codecov Report

youknowriad commented Aug 7, 2017

youknowriad commented Aug 9, 2017

jasmussen commented Aug 9, 2017

youknowriad commented Aug 9, 2017 • edited Loading

youknowriad commented Aug 9, 2017

jasmussen commented Aug 9, 2017

youknowriad commented Aug 9, 2017

jasmussen commented Aug 9, 2017

youknowriad commented Aug 9, 2017

jasmussen commented Aug 9, 2017

mtias commented Aug 9, 2017

youknowriad commented Aug 9, 2017

mtias commented Aug 9, 2017

youknowriad commented Aug 9, 2017

jasmussen commented Aug 9, 2017

youknowriad Aug 4, 2017 •

edited

Loading

youknowriad commented Aug 4, 2017 •

edited

Loading

mtias commented Aug 4, 2017 •

edited

Loading

youknowriad commented Aug 4, 2017 •

edited

Loading

codecov bot commented Aug 4, 2017 •

edited

Loading

youknowriad commented Aug 9, 2017 •

edited

Loading