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

Fix issues with image initial size #11846

Merged
merged 3 commits into from Nov 15, 2018
Merged

Fix issues with image initial size #11846

merged 3 commits into from Nov 15, 2018

Conversation

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Nov 14, 2018

This PR aims to fix an issue where if an editor style provides a wider main column width than the default, images are still constrained by the default editor width (580px).

For example, the editor main column width in TwentyNineteen is wider than that of the vanilla editor style. So any image you insert there is constrained, but only in the editor.

This PR simply removes our max-width variable and defaults it to 100%. I can't tell if there are any downsides or risks to this, please give me your thoughts on it.

Before:

before

After:

after

closes #9302

@jasmussen jasmussen added this to the 4.4 milestone Nov 14, 2018
@jasmussen jasmussen self-assigned this Nov 14, 2018
@jasmussen jasmussen requested a review from Nov 14, 2018
@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 14, 2018

Not sure what I'm doing wrong but I can't resize the images this way.

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 14, 2018

Hmm, indeed.

It works if I remove both the min and maxwidths entirely, but then you can size the image above the width of the container. Investigating.

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 14, 2018

So, in 3501d96 I committed a horrible hack just to get it working.

To be clear, we can't ship that. Not only is it terrible, it's likely to break in a billion ways.

However, hopefully it can suggest what we need to get to a proper solution. I'm not actually sure why the 100% property didn't work — that would've been way cleaner. Any help appreciated.

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 14, 2018

Unless we can find a good way to make the max-width match that of the theme, honestly I feel like we should remove the max-width and minwidth constraints entirely:

resize beyond max

Yeah, that's not ideal. But at this point it's better given how broken TwentyNineteen and any other theme that provides an editor style is.

The classic editor also doesn't have a max width.

@notnownikki
Copy link
Member

@notnownikki notnownikki commented Nov 14, 2018

I'm in favor of getting rid of max width in theory, if we have plans in the future of using Gutenberg in a theme's context for site building etc., then max width is going to bite us there.

(to be honest, I'm not sure Gutenberg should be defining widths at all if the current theme can provide them, but that's a different issue 😁 )

Couple of things:

  1. Portrait images can bleed out of the block and cover other blocks and the caption:

imagebleed

Image used: https://i.imgur.com/AWJ2njK.jpg

  1. Switching to the narrow screen view leaves a load of space around resized images:

Insert a landscape aspect image, resize it to the width of the editor, then resize your window to switch to the narrow screen styles.

weirdresize

Image used: https://i.imgur.com/vu0EZ38.gif

@kjellr
Copy link
Contributor

@kjellr kjellr commented Nov 14, 2018

Thanks for taking a look at this! I'm excited to get it fixed. Your hack does work for the most part, but I'm seeing it fall apart at smaller screen sizes:

images

I wonder if the fix for this might end up being related to #5650 and #6177. Seems like this could tie into the theme's content width in some way.

@chrisvanpatten
Copy link
Member

@chrisvanpatten chrisvanpatten commented Nov 14, 2018

Yeah might be worth flagging in @azaozz who has spent a ton of time thinking about how we handle images and such in Gutenberg :)

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 14, 2018

To be clear, the PR as implemented now has more or less fallen apart and can't really be merged as is, so it's more of a starting point for discussing the actual fix.

Which, as much as I'd like for it not to be necessary, might require use of the $content_width variable.

@azaozz
Copy link
Contributor

@azaozz azaozz commented Nov 14, 2018

This PR simply removes our max-width variable and defaults it to 100%.

Tried that too and didn't see any downsides. Still thinking that ideally that should always be set by the theme. Themes with fixed width for the main column may have unexpected results.

Which, as much as I'd like for it not to be necessary, might require use of the $content_width variable.

Yeah, I was really hoping we can avoid using that. Been looking at ways to replace it, but we need a simple, straightforward method for three-way communication between the theme, the editor, and the front-end.

In the (experimental) #11377 looked at introducing another "global" setting block_width (https://github.com/WordPress/gutenberg/pull/11377/files#diff-ad1f24a74f64a1c137bfb28da9349bd1R31) which is an array with three values. That would act more like max-width for the three possible widths: default, wide, and full. This is "workable" although for themes like TwentyNineteen it may need some additional considerations.

In any case, themes should be setting their own sizes attributes (by using the core filter). That's the main enhancement in the image block PR: to let themes do more with images on the front-end.

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 14, 2018

Tried that too and didn't see any downsides. Still thinking that ideally that should always be set by the theme. Themes with fixed width for the main column may have unexpected results.

Well, the downside is that the resizablebox component we're using for this doesn't appear to work there. The initial size is right, but otherwise the thing isn't resizable at all.

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 14, 2018

So the ideal solution is to figure out why ResizableBox isn't able to handle the 100% value. Because a theme might have a 50% wide main column, so the content-width variable is a little useless in that situation regardless. Barring that, leveraging the column width will at least go a ways to fixing this.

I did notice the reports Nikki noted, that especially in the initial sizing up of portrait images, the sizing is a little funky. But this seems to settle after a bit. Figuring that funk out might be a different fix.

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 14, 2018

So the ideal solution is to figure out why ResizableBox isn't able to handle the 100% value

I don't know if it's capable for handling that or not but even if it's capable, it won't work because the width of the container of the block will follow the image size, which means 100% will decrease as soon as you resize the image to a lower size and you won't be able to increase the size of the image. (for floats at least)

Joen Asmussen added 2 commits Nov 15, 2018
This PR aims to fix an issue where if an editor style provides a wider main column width than the default, images are still constrained by the default editor width (580px).

For example, the editor main column width in TwentyNineteen is wider than that of the vanilla editor style. So any image you insert there is constrained, but only in the editor.

This PR simply removes our max-width variable and defaults it to 100%. I can't tell if there are any downsides or risks to this, please give me your thoughts on it.
This commit uses a horrendous hack to calculate the width that the image should have as its max-width. It needs to be refactored, better, don't merge.

However it works.
@jasmussen jasmussen force-pushed the fix/image-content-width branch from 3501d96 to 6d81175 Nov 15, 2018
With the current implementation of ResizableBox, an image needs an explicit pixel value for the max-width.
In absence of being able to set the content-width, this max-width is currently dictated by the vanilla editor style.
The following variable adds a buffer to this vanilla style, so 3rd party themes have some wiggleroom.
This does, in most cases, allow you to scale the image beyond the width of the main column, though not infinitely.
It would be good to revisit this once a content-width variable becomes available.
@jasmussen jasmussen force-pushed the fix/image-content-width branch from 6d81175 to dfdd839 Nov 15, 2018
@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 15, 2018

Okay, rebased and pushed a change that I think we might be able to ship.

The change is to use the vanilla editor width (580px), but with a 2.5x buffer. This means:

  • Images inserted fresh in TwentyNineteen and other themes that have a wider than 580px editor width, will look right.
  • Images can be sized wider than the main column, but not infinitely so.

I've added a comment explaining this, and a todo so we can revisit in the future. But for now, this seems like a better experience than in master, and worth shipping.

Some GIFs:

max-width

Portrait:

portrait

Note that we can actually "fix" it so that the gray boundaries scale with the image:

displaytable

However that requires setting the image block to display: table;, like so:

.editor-block-list__block[data-type="core/image"] {
    display: table;
}

displaytable

However I would recommend us not doing that, as it has side effects that on one hand might not be worth it, and on the other hand are definitely too risky to merge this late in the process:

screenshot 2018-11-15 at 09 28 51

screenshot 2018-11-15 at 09 28 56

In any case, I feel like the implementation added in dfdd839 is a good balanced fix for now, and I think we should merge it for 4.4. Your thoughts and reviews are appreciated.

@youknowriad youknowriad removed this from the 4.4 milestone Nov 15, 2018
@youknowriad youknowriad added this to the 4.5 milestone Nov 15, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

I feel this is one of those issues where there's no ideal solution. I think the current proposal is a good enhancement so I'm approving.

@youknowriad youknowriad removed this from the 4.5 milestone Nov 15, 2018
@youknowriad youknowriad added this to the 4.4 milestone Nov 15, 2018
@jasmussen jasmussen merged commit d970375 into master Nov 15, 2018
1 check passed
@jasmussen jasmussen deleted the fix/image-content-width branch Nov 15, 2018
@azaozz
Copy link
Contributor

@azaozz azaozz commented Nov 15, 2018

Uh, sorry I'm late to comment here, so much things to do, so little time... :)

Tried looking at why ResizableBox can't handle "100%" size images. Seems structural CSS related, but needs a lot of changes to fix.

Came up to pretty much the same conclusion: it needs a fixed width but we can make that width pretty large to not limit the theme.

Another thing I'd like to test is setting the container's max-width to the image file's actual width. That seems to work pretty well (fixes ResizableBox) and prevents upscaling (currently you can upscale freely).

(BTW, WOW! GH? What is that page jumping!? Large images and not having width/height??) :)

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Nov 15, 2018

Lots of improvements still to look into, here, sure.

(BTW, WOW! GH? What is that page jumping!? Large images and not having width/height??) :)

So just to stop myself from having a heart attack, are you referring to this github page loading? Or are you referring to this branch loading on your local dev system?

@azaozz
Copy link
Contributor

@azaozz azaozz commented Nov 15, 2018

Uh, sorry about that, it's this Github page. There are several large images that load slow(er), and the page keeps jumping and pushing the comment form under the fold... Quite annoying.
(We shouldn't let that happen in WP) :)

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this issue Nov 22, 2018
* Fix issues with image initial size

This PR aims to fix an issue where if an editor style provides a wider main column width than the default, images are still constrained by the default editor width (580px).

For example, the editor main column width in TwentyNineteen is wider than that of the vanilla editor style. So any image you insert there is constrained, but only in the editor.

This PR simply removes our max-width variable and defaults it to 100%. I can't tell if there are any downsides or risks to this, please give me your thoughts on it.

* Create proof of concept instead.

This commit uses a horrendous hack to calculate the width that the image should have as its max-width. It needs to be refactored, better, don't merge.

However it works.

* Use buffered max-width.

With the current implementation of ResizableBox, an image needs an explicit pixel value for the max-width.
In absence of being able to set the content-width, this max-width is currently dictated by the vanilla editor style.
The following variable adds a buffer to this vanilla style, so 3rd party themes have some wiggleroom.
This does, in most cases, allow you to scale the image beyond the width of the main column, though not infinitely.
It would be good to revisit this once a content-width variable becomes available.
grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this issue Jan 4, 2019
* Fix issues with image initial size

This PR aims to fix an issue where if an editor style provides a wider main column width than the default, images are still constrained by the default editor width (580px).

For example, the editor main column width in TwentyNineteen is wider than that of the vanilla editor style. So any image you insert there is constrained, but only in the editor.

This PR simply removes our max-width variable and defaults it to 100%. I can't tell if there are any downsides or risks to this, please give me your thoughts on it.

* Create proof of concept instead.

This commit uses a horrendous hack to calculate the width that the image should have as its max-width. It needs to be refactored, better, don't merge.

However it works.

* Use buffered max-width.

With the current implementation of ResizableBox, an image needs an explicit pixel value for the max-width.
In absence of being able to set the content-width, this max-width is currently dictated by the vanilla editor style.
The following variable adds a buffer to this vanilla style, so 3rd party themes have some wiggleroom.
This does, in most cases, allow you to scale the image beyond the width of the main column, though not infinitely.
It would be good to revisit this once a content-width variable becomes available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants