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 ability to manually set image dimensions #5812

Merged
merged 12 commits into from Apr 18, 2018

Conversation

Projects
None yet
9 participants
@noisysocks
Member

noisysocks commented Mar 27, 2018

Description

Closes #4914.

Let users manually set the width and height of an image either using a text field or by a preset percentage amount.

How Has This Been Tested?

  1. Insert an image block
  2. Open the block inspector
  3. Resize the image using the preset % amounts (e.g. 25%)
  4. Resize the image by manually entering a width and height
  5. Resize the image by using the drag handles
  6. Click Reset

In all cases, the width value, the height value, and the size of the image in the editor should reflect the entered size.

Screenshots (jpeg or gifs if applicable):

dimensions

@noisysocks

This comment has been minimized.

Member

noisysocks commented Mar 27, 2018

This differs from the design a little: instead of the Dimensions label, I made the width and height fields each have its own label. It's generally good a11y practice for every input to have a label.

Let me know if you hate it and I'll figure out a way to have labels for each field but still have the UI look like the mockup! 🙂

cc. @jasmussen

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 27, 2018

Nice, thank you for working on this! CC: @karmatosed

First thought, the pill button is a really nice interface for this.

GIF:

dimensions

Some issues:

  • The intrinsic dimensions (i.e. what you see when you press Reset) aren't grayed enough. Keep in mind these dimensions refer to the full size of the image, but aren't yet set.
  • Sizes 25% and 50% seem to map to 75% and 100%, whereas 75% and 100% make the image block itself (see the scrollbar) way larger than the editing canvas itself. This was tested on a non retina screen. Keep in mind, if an image is 300x200, then the 100% dimensions are 300x200, and the 50% dimensions are 150x100. Smart retina support is something we could explore in the future but shouldn't be part of V1.
  • The image dimensions for 75% and 100% aren't actually set at all. The image isn't sized up. Similarly if I type in a weird dimension that should stretch the image, it doesn't work. Could this be some flex box stuff, or some object-fit CSS stuff messing things? I can look in this branch in the inspector later perhaps if you need help debugging.

Otherwise, really nice work, and good interface.

@Luehrsen

This comment has been minimized.

Contributor

Luehrsen commented Mar 27, 2018

Just a thought: If we offer the user an easy way to edit the image dimensions, they will use that and potentially break a lot of designs without knowing why. I believe, that this can be very hurtful in a responsive situation.

Has there been any consideration to that? Otherwise, great work! 👍

@karmatosed

This comment has been minimized.

Member

karmatosed commented Mar 27, 2018

Just a thought: If we offer the user an easy way to edit the image dimensions, they will use that and potentially break a lot of designs without knowing why. I believe, that this can be very hurtful in a responsive situation.

@Luehrsen whilst I can see the concern there, I think it's important to balance giving control. In some cases yes issues could happen, in many this won't be the case. Another point to note is responsive wise as long as we handle images and also the theme does properly, this shouldn't cause issues - fixed !important widths is where they fall over. It's good to be mindful but this is a feature people want and safe enough to empower.

@noisysocks great work on this, really cool to see it taking shape.

@jasmussen I agree with greying out actions on start, if you can't do then don't show active.

Smart retina support is something we could explore in the future but shouldn't be part of V1.

That would be amazing :)

@noisysocks

This comment has been minimized.

Member

noisysocks commented Mar 28, 2018

Sizes 25% and 50% seem to map to 75% and 100%, whereas 75% and 100% make the image block itself (see the scrollbar) way larger than the editing canvas itself.

My mistake. I misinterpreted #4914 (comment) and thought that intrinsic dimensions meant the original size of the image file, i.e. what you see if you select the file on your computer and press ⌘I.

I've updated this PR so that clicking e.g. 50% sets the width and height to half of the size of the image as it appears in the visual editor.

Note for code reviewers: Doing this involved re-arranging a bunch of code so as to pull the <ImageSize> up to a higher level. I've tried to neaten it all up by splitting render() into some smaller methods.

The intrinsic dimensions (i.e. what you see when you press Reset) aren't grayed enough.

I've changed it to a lighter grey. Should we also change the colour we use for other input placeholders throughout Gutenberg (or wp-admin)? e.g. Search for a block in the inserter and Add New Tag in the document settings

Similarly if I type in a weird dimension that should stretch the image, it doesn't work. Could this be some flex box stuff, or some object-fit CSS stuff messing things? I can look in this branch in the inspector later perhaps if you need help debugging.

I could use some help with this. It's to do with how we set width: 100%; height: auto on our images in the editor and in the frontend, but I'm struggling to come up with a fix that allows for stretched images that are also responsive and work in different themes.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 28, 2018

I've updated this PR so that clicking e.g. 50% sets the width and height to half of the size of the image as it appears in the visual editor.

👍 👍

I've changed it to a lighter grey. Should we also change the colour we use for other input placeholders throughout Gutenberg (or wp-admin)? e.g. Search for a block in the inserter and Add New Tag in the document settings

I was indeed referring to the placeholder text. And yes, ideally any changes we make here we make across all input form fields in Gutenberg. The color looked so dark it didn't look like placeholder text to me. But given the fact that it was placeholder text, perhaps it's best to address this separately?

I'm currently working on some unification in #5605, perhaps you can revert your fix and I can revisit in that PR?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 28, 2018

I could use some help with this. It's to do with how we set width: 100%; height: auto on our images in the editor and in the frontend, but I'm struggling to come up with a fix that allows for stretched images that are also responsive and work in different themes.

These are the inline styles we output, correct? This keeps coming back to bite us. CC: @mtias — I'd love for us to be able to rid of the inline styles, or at least find a way to add nuance here. #5460 is one attempt at improving things for floats. There's also discussion in #5706 (review).

A plan B is to simply unset those styles, as soon as you explicitly set the dimensions using this tool.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Mar 29, 2018

I'm currently working on some unification in #5605, perhaps you can revert your fix and I can revisit in that PR?

👌 done.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 29, 2018

Had a quick look at this branch, nice work, it's nearly there.

I just pushed a fix that adds the following:

.wp-block-image {
	[...]
	&.is-resized img {
		height: 100%;
	}
	[...]
}

The problem used to be that if you manually set a width or height that broke the intrinsic aspect ratio of the image (say setting a size of 100x100 on a 300x200 image), this would not work. This is because using the resizing tool we use, these properties are set on the parent div to the image, not the img itself. Not only that, but we inherit a height: auto; property that is commonly used to make images responsive.

The change I just made in c731101 overrides that if an image is resized, i.e. the image has the is-resized class:

works

Note how it works right up until the end. This is because if you resize using the drag handles, then is-resized is applied. If you resize using the pillbox control, then is-resized is applied. All good.

The only missing piece is that if you manually set the dimensions on a fresh image (i.e. insert new image, don't resize using handles or pillbox, then set dimensions explicitly), then is-resized is not set. Which it should be.

As such, the remaining step is to do that — set is-resized when you input dimensions. @noisysocks can you help me do that?

@paulwilde

This comment has been minimized.

Contributor

paulwilde commented Mar 29, 2018

Just throwing ideas out there, but it would be nice if next to the Width / Height inputs (Right side of Height?) was a little lock icon (With a tooltip with something like 'Lock/Unlock Aspect Ratio') and when pressed it would enable/disable locking of the aspect ratio for the image. So if you were to change the width or height, then the other input would automatically update to retain the aspect ratio of the image.

I know from past experiences with clients who adjust the size of images, lack of such a feature does tend to produce some stretched out images for no reason.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 29, 2018

Yep, love that idea Paul, had similar thoughts. But such an enhancement should probably be separate from this PR.

@paulwilde

This comment has been minimized.

Contributor

paulwilde commented Mar 29, 2018

Agreed it should be an additional PR. Just wanted to make sure it was a consideration. :)

render() {
const { attributes, setAttributes, isSelected, className, settings, toggleSelection } = this.props;
const { url, alt, caption, align, id, href, width, height } = attributes;
renderBlockControls() {

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Contributor

Minor: Just wanted to share my concerns about this pattern: (render*) functions. I think this probably indicates that we need to split this into smaller components. I think this pattern makes it hard to maintain the component without having an obvious pro.

This comment has been minimized.

@mtias

mtias Apr 2, 2018

Contributor

Also, this is something we'd want to be consistent across core blocks, because people will learn from them.

This comment has been minimized.

@noisysocks

noisysocks Apr 3, 2018

Member

Yeah, I also don't very much like this pattern. I opted for it instead of splitting the block into smaller components because there was a lot of codependency on local state. Looking at it now with fresh (post-Easter) eyes, though, it doesn't look like it would be so bad to pass props around. Watch this space...

@youknowriad

Code wise, this is looking good for me

@paulwilde

This comment has been minimized.

Contributor

paulwilde commented Mar 30, 2018

I've noticed a slight visual bug in Safari (Seems fine in Chrome) where the placeholder text inside the width/height inputs is different than actual input text.

This seems to due to core forcing line-height: 1 on input[type=number]. Removing that line-height actually fixes the flickering of the line-height for me whilst also having no negative implications in Chrome.

Adding line-height: 1.25 onto the following rule you've added fixes the issue for me.

.blocks-image-dimensions__width,
.blocks-image-dimensions__height {
    margin-bottom: 0.5em;
}

Other than that, it looks and works great.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Apr 3, 2018

As such, the remaining step is to do that — set is-resized when you input dimensions. @noisysocks can you help me do that?

Thanks for looking into this @jasmussen. I've made it so that:

  • is-resized is set if a width or height is manually entered.
  • on mobile, the <img /> in wrapped in a div that has a width and height style set. This makes the .is-resized img { height: 100% } rule work on mobile devices.
  • you cannot manually enter a width and height if the image is set to full or wide alignment. This makes it consistent with the resize behaviour.

I think everything is looking great on the editor side of things! 🎉

On the frontend, though, we have the same issue where images ignore any stretched aspect ratio that you enter. We can't do what we do in the editor (set an explicit width and height style) since we do not know the width of the page. Thoughts?

Adding line-height: 1.25 onto the following rule you've added fixes the issue for me.

Thanks @paulwilde! I've added your fix in dea294d.

@mtias

This comment has been minimized.

Contributor

mtias commented Apr 3, 2018

@noisysocks see the implementation of font-sizes for potential inspiration. We could set classes for the fixed percentage values and size with CSS relative to container. If user chooses a custom value we'd save as inline width/height.

Also cc @azaozz for thoughts on relationship with responsive images.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 10, 2018

Valiant work.

This works mostly well. I think it's 99% there. But there are still some edgecases, and I think we actually need to get #5973 merged in first, then rebase this PR off of that. Because right now in the editor you are setting the width of the figure element with inline styles, and this leads to some strange edgecases when you resize the viewport,
screen shot 2018-04-10 at 10 16 49

Also recorded a GIF that shows some iffiness.

The key here is that so long as we set inline dimensions on the figure, things blow up in strange ways. If we merge in #5973 first, the figure will have width: fit-content;, which seemed to fix up your branch and clear any edgecases for me, even as I just did it manually in the inspector.


The good news is that overall, you got the behavior right. The pill button is a shortcut for filling out the image dimensions:

megaman

By the way can you add an "Image Dimensions" label? Something like this:

screen shot 2018-04-10 at 10 22 05

Perhaps Bold as well.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Apr 11, 2018

Thanks @jasmussen. I've cleaned up this PR and added the label you requested. Let's wait for #5973 before continuing 🙂

noisysocks added some commits Mar 27, 2018

Add ability to manually set image dimensions
Let users manually set the width and height of an image either using a
text field or by a preset percentage amount.
Fix Safari misaligning width/height placeholders
Setting an explicit line height on the width and height text fields
prevents a small visual bug in Safari where the fields flicker when a
number is typed in.
Fix paragraph and image inspector control margins
- Fixes Reset button being too tall on mobile in the paragraph and image
  inspectors
- Makes the margin at bottom of a <ButtonGroup> consistent with all
  other controls (1em)
- Fix regression causing width and height fields from being too close to
  each other

noisysocks and others added some commits Apr 11, 2018

Add explicit height to the editor image only
The resizing tools we use size a parent div on the image. The height:
auto responsive CSS we inherit from up higher messes with this. So this
push adds an explicit 100% height on resized images.

Co-authored-by: Joen Asmussen <joen@automattic.com>
Don't set inline styles on an image's <figure>
Let the <figure> get its width from the fit-content rule that was added
in 2dc9fb3.
@noisysocks

This comment has been minimized.

Member

noisysocks commented Apr 16, 2018

Hey @jasmussen. I pulled in #5973, removed the inline <figure> styles, and fixed some glitchy resize behaviour by updating re-resizble. Would you mind running through this again?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 16, 2018

Very nice work. This is 99% there. Floats work, resizing works, shortcuts work, obscure manually typed dimensions work.

I did encounter one Editor-only issue, which is a little hard to explain, here's a GIF. Before you watch, though — I'm pretty sure it's my fault, and this issue is in master also. Going to look in a minute. GIF:

resize

It looks like if an image is inserted fresh, in the editor no explicit dimensions are set on it. This is effectively the same as what the Reset button should do, completely untouched, meaning that if you resize the viewport our Editor responsive styles kick in and resize the image if need be.

However at some point, and it appears to be as soon as you interact with the image in some way, a bunch of inline styles are set on the image — again, editor only. This means that even if you later on press the "Reset" button, those styles are still present in the editor, misrepesenting the image dimensions. For example try these steps to reproduce:

  1. Make the viewport thin
  2. Insert an image that is larger than the viewport, notice that the responsive styles correctly kick in and the image correctly resizes to the editor responsive styles. You can inspect the image and see there are no inline styles on any divs or figures.
  3. Interact with the image, for example resize it a bit, then open the inspector and press the Reset button. You can now inspect the image and see that even though no explicit dimensions are set, there are inline style.
  4. If you now resize the viewport to be large, you'll see that the image is still small, even though it should scale to fit (because no dimensions are set)

It's interesting that this issue is surfaced now — I think it's due to the fit-content property I introduced. It seems like our resizing library sets the dimensions on the div instead of the img:

screen shot 2018-04-16 at 09 18 55

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 16, 2018

Given that the issue I discovered seems to be separate, I opened a ticket for it. And unless @youknowriad objects, we should probably address it separately: #6191. In other words, I think this branch is good to go.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Apr 17, 2018

Thanks @jasmussen! Agree we should look into that issue separately—thanks for ticketing.

@youknowriad—would you mind glancing over this? The code has changed a little bit since you last reviewed it.

@@ -220,10 +224,69 @@ class ImageBlock extends Component {
onChange={ this.updateImageURL }
/>
) }
<div className="blocks-image-dimensions">

This comment has been minimized.

@youknowriad

youknowriad Apr 17, 2018

Contributor

I feel that these classNames don't follow our guidelines, should it be blocks-image__dimentions?

This comment has been minimized.

@noisysocks
label={ __( 'Width' ) }
value={ width !== undefined ? width : '' }
placeholder={ selectedSize.width }
onChange={ ( value ) => {

This comment has been minimized.

@youknowriad

youknowriad Apr 17, 2018

Contributor

Minor: I know this is not the first usage of arrow functions as props here but since this is already a class, we could use bound methods instead.

This comment has been minimized.

@noisysocks
@@ -239,7 +302,11 @@ class ImageBlock extends Component {
const img = <img src={ url } alt={ alt } onClick={ this.onImageClick } />;
if ( ! isResizable || ! imageWidthWithinContainer ) {
return img;
return (
<div style={ { width, height } }>

This comment has been minimized.

@youknowriad

youknowriad Apr 17, 2018

Contributor

This changes feels a bit weird, we're adding an unnecessary extra div for me and I believe this div is not present in the "save" representation? Can we avoid this extra div? Why can't we add these styles to the "figure"?

This comment has been minimized.

@noisysocks

noisysocks Apr 18, 2018

Member

This changes feels a bit weird, we're adding an unnecessary extra div for me and I believe this div is not present in the "save" representation?

Our CSS rules (namely .wp-block-image.is-resized img { height: 100% }) rely on the img being contained in something that has an explicit width and height. On desktop, this is true because re-resizable wraps the image in a div with an inline width and height. On mobile, however, we don't load re-resizable and so our CSS rules don't work.

By wrapping the img with our own div on mobile, our CSS rules work on both mobile and desktop because our markup has a similar structure.

Why can't we add these styles to the "figure"?

We can't set a height on the figure because we do not know how tall the caption underneath the image is.

@youknowriad

Left some comments but nothing really blocking. This is in a good shape.

@@ -5,12 +5,12 @@ import BaseControl from '../base-control';
import withInstanceId from '../higher-order/with-instance-id';
import './style.scss';
function TextControl( { label, value, help, instanceId, onChange, type = 'text', ...props } ) {
function TextControl( { label, value, help, className, instanceId, onChange, type = 'text', ...props } ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 17, 2018

Member

Nice catch we should have added this prop since the start 👍 We should also add the className prop to the component readme.

This comment has been minimized.

@noisysocks
margin-bottom: 1em;
.blocks-image-dimensions__row {
display: flex;

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 17, 2018

Member

Changes to flex layout are always risky in IE11, but in my IE tests things are looking fine :)

This comment has been minimized.

@noisysocks

noisysocks Apr 18, 2018

Member

😅 thanks for checking!

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Apr 17, 2018

This is looking great, and tested well 👍
There is one improvement I think should happen not certain if in this PR or we can create a follow-up PR. If I add an image and I set it to the 50%, and later I change the image, the new image is not 50% but contains the fixed dimension of the previous image. If the proportions are different the image looks messy. I think this can be fixed if we use a class for relative sizes. Meanwhile, one option would be to remove the dimensions when we change the image, like what we do when we change the size on the select box.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 17, 2018

one option would be to remove the dimensions when we change the image, like what we do when we change the size on the select box.

This seems like the right thing to do.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Apr 18, 2018

Thanks @youknowriad and @jorgefilipecosta!

one option would be to remove the dimensions when we change the image, like what we do when we change the size on the select box.

This seems like the right thing to do.

👍 af073ab

@noisysocks noisysocks merged commit 5a6fe0d into master Apr 18, 2018

2 checks passed

codecov/project 43.86% (-0.5%) compared to 0de347f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the add/image-dimensions branch Apr 18, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 18, 2018

🔥

nuzzio added a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018

Add ability to manually set image dimensions (WordPress#5812)
* Add ability to manually set image dimensions

Let users manually set the width and height of an image either using a
text field or by a preset percentage amount.

* Reset image width and height when source type changes

* Fix Safari misaligning width/height placeholders

Setting an explicit line height on the width and height text fields
prevents a small visual bug in Safari where the fields flicker when a
number is typed in.

* Fix paragraph and image inspector control margins

- Fixes Reset button being too tall on mobile in the paragraph and image
  inspectors
- Makes the margin at bottom of a <ButtonGroup> consistent with all
  other controls (1em)
- Fix regression causing width and height fields from being too close to
  each other

* Add explicit height to the editor image only

The resizing tools we use size a parent div on the image. The height:
auto responsive CSS we inherit from up higher messes with this. So this
push adds an explicit 100% height on resized images.

Co-authored-by: Joen Asmussen <joen@automattic.com>

* Add 'Image Dimensions' label above width and height controls

* Don't set inline styles on an image's <figure>

Let the <figure> get its width from the fit-content rule that was added
in 2dc9fb3.

* Update re-resizable to v4.4.8

* Rename blocks-image-dimensions → blocks-image__dimensions

* Use class methods instead of calling setAttributes() directly

* Add className prop to TextControl README

* Reset any set image dimensions when image is changed

If the user selects a new image, forget any image dimensions that they
added since it likely no longer makes any sense.
className="blocks-image__dimensions__width"
label={ __( 'Width' ) }
value={ width !== undefined ? width : '' }
placeholder={ selectedSize.width }

This comment has been minimized.

@aduth

aduth Jul 18, 2018

Member

This can apparently be unassigned, resulting in warnings logged to the console.

Issue at #8026.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment