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 sizes grow outside of block container #19424

Closed
karmatosed opened this issue Jan 6, 2020 · 7 comments
Closed

Image sizes grow outside of block container #19424

karmatosed opened this issue Jan 6, 2020 · 7 comments
Labels
[Block] Image Affects the Image Block Needs Dev Ready for, and needs developer efforts [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later

Comments

@karmatosed
Copy link
Member

I am not sure if this is expected behaviour but I found it a little weird. If you pick a large size image (or full width), by changing percentage you can end up with it popping out of the block container.

image

My expectation would be that it fits inside, but this might be ok as you are selecting a wider image that is bigger than container.

@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label Jan 6, 2020
@mapk
Copy link
Contributor

mapk commented Jan 7, 2020

This was discussed in today's Gutenberg Triage by the Design Team in slack.

The suggestion is to limit the expansion of the image to within the block's edge (don't let it expand outside the block). This way it doesn't look broken. If the user would like to expand the image further, they can set the block to wide or full width. This feels like a better solution than allowing content to protrude outside the block.

@mapk mapk added [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later Needs Dev Ready for, and needs developer efforts and removed Needs Design Feedback Needs general design feedback. labels Jan 7, 2020
@Addison-Stavlo
Copy link
Contributor

Taking a look at some of the code that is causing this:

// 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.
// @todo It would be good to revisit this once a content-width variable becomes available.
const maxWidthBuffer = maxWidth * 2.5;

How do you feel about the reasoning above for why this is allowed to be resized larger than the parent block? Essentially: so 3rd party themes have some wiggleroom... to scale the image beyond the width of the main column.

If we updated the resizable to use maxWidth instead of maxWidthBuffer this would stop the resizer from allowing the user to expand the image outside its block. Without changing this we can still limit the max-width of the children inside the block, but the resizer will still try to resize the image up to that larger maxWidthBuffer width, causing it to stretch vertically once it reaches the max-width restriction. So currently the best option looks like removing this 'wiggleroom' if it is agreed that it is not necessary to keep for 3rd party theme development.

@mapk commented above:

If the user would like to expand the image further, they can set the block to wide or full width.

Agreed, and maybe this is enough for 3rd party themes as well? Or do we need to worry about keeping this maxWidthBuffer 'wiggleroom' for them (in which case we might have to figure out some sort of tricky way to limit the resizer)?

@mapk
Copy link
Contributor

mapk commented Jan 8, 2020

Excellent digging into this, @Addison-Stavlo! Thanks for sharing this finding. 😄

So currently the best option looks like removing this 'wiggleroom' if it is agreed that it is not necessary to keep for 3rd party theme development.

I'm all for this, but I don't know enough about 3rd party theme development to make the call. Let's get some thoughts from @allancole, @kjellr, and @karmatosed on this one before going further.

@karmatosed
Copy link
Member Author

As we are only limiting in editor this won't be an issue with it as a theme can override in an editor style.

@Addison-Stavlo
Copy link
Contributor

As we are only limiting in editor this won't be an issue with it as a theme can override in an editor style.

The theme editor style override is done through css only? I am guessing they don't have the capability of altering the maxWidth prop that is passed into the ImageEdit component?

While a theme can override css, this maxWidth or maxWidthBuffer is not a css rule that we can override in this manner. It is a javascript variable passed into the 3rd party resizer component so it can create some upper limit on the amount the image can be resized.

If the resizer is fed a 'maxWidth' of 600, it won't matter if a theme overrides the CSS of the column and image block to be 800, the resizer will still be limited to 600. Similarly if the resizer is fed a maxWidthBuffer of 1400, it won't matter if the default column and image block has a css max-width of 600, it will still try to resize the image beyond that since it has the max resizing width of 1400.

Unless a 3rd party theme has the capability of directly changing the maxWidth prop that is fed into the ImageEdit component, Its sounding like we may need to iterate on changing this maxWidthBuffer to be less of a buffer and more of a specific value based on the current width settings on the parent blocks somehow. Meaning we may need to start with an initial buffer, and then set it to a specific value after the dom has been rendered and we can get the parents values. Perhaps there are other ideas on how to get around this as well?

@Addison-Stavlo
Copy link
Contributor

we may need to start with an initial buffer, and then set it to a specific value after the dom has been rendered and we can get the parents values.

The PR mentioned above does this, it seems to be working well and should support changes made by 3rd party themes.

@youknowriad youknowriad added the [Block] Image Affects the Image Block label Jan 17, 2020
@youknowriad
Copy link
Contributor

This is a duplicate of #12168

Can we consolidate discussions there?

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 Needs Dev Ready for, and needs developer efforts [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later
Projects
None yet
Development

No branches or pull requests

4 participants