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

Remove max-width setting on image-block #14911

Merged
merged 1 commit into from Nov 22, 2019
Merged

Conversation

@m-e-h
Copy link
Member

m-e-h commented Apr 10, 2019

Description

I'm not seeing max-width set for any other blocks.
This seems like a job for the align classes.

Also, it overrides my theme's article > * { max-width: $width; }

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
This seems like a job for the align classes. 
I'm not seeing `max-width` set for any other blocks.

Also, it overrides my theme's   `article > * { max-width: $width; }`
@m-e-h m-e-h requested a review from chrisvanpatten as a code owner Apr 10, 2019
@m-e-h

This comment has been minimized.

Copy link
Member Author

m-e-h commented Apr 11, 2019

Oops. Sorry for the crappy branch name. I created it directly in GitHub.
Should I delete this PR and do a new one?

@@ -1,6 +1,4 @@
.wp-block-image {
max-width: 100%;

This comment has been minimized.

Copy link
@m-e-h

m-e-h Apr 12, 2019

Author Member

Maybe this was meant for editor.css. Just noticed that if I give .wp-block-image a non-100% max-width for the front-end, that it will override this and break the wide and full images in the editor.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 30, 2019

Member

It seems like we could remove this style, in my tests I'm not noticing different behavior in 2019 theme when this style is set. Any thoughts on this @jasmussen, @kjellr?

This comment has been minimized.

Copy link
@kjellr

kjellr May 30, 2019

Contributor

Yeah, I think it's generally safe to remove this. Things seem to work fine without it on my end — tested in Twenty Nineteen, Twenty Thirteen, and the Gutenberg Starter theme.

It's true that if a theme were to set this to anything other than max-width: 100% in the editor, it'd break the editor's wide and full alignments. (@m-e-h, is that what you're referring to above?) But that would be the theme dev's decision to break things, so it'd be up to them to fix it. Everything seem to work fine if this style is removed out of the box.

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Nov 2, 2019

@m-e-h. Hey Marty. Can we get a status update on this PR? Thanks.

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Nov 19, 2019

Both @jorgefilipecosta and @kjellr tested and found no issues with removing the CSS setting, however @m-e-h apparently is unsure :) Marking this as stale.

@m-e-h

This comment has been minimized.

Copy link
Member Author

m-e-h commented Nov 19, 2019

Haha! Sorry @paaljoachim & @draganescu !
I do still think that the max-width on the image block is unnecessary.
I don't think it's bothering anybody at this point but it really isn't serving a purpose.

What would you like for a status update? A repo sync/update on the PR and some testing?

I'm fine with closing this too. Whatever you all think.

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Nov 19, 2019

My logical brain is not functioning right now. So just go with whatever you think Marty..:)
@m-e-h

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 20, 2019

Pretty sure the initial reasoning for this to have been added was this: https://www.w3schools.com/howto/tryit.asp?filename=tryhow_css_image_responsive2

That is, classic responsive images that don't scale beyond 1) the viewport, 2) the intrinsic dimensions of the image.

However in this case it looks like it's applied to the block, not the img, so probably totally fine to remove. 👍 👍

Copy link
Contributor

draganescu left a comment

Given that no one found any issues and @jasmussen gave some thumbs up this looks good to go :)

@draganescu draganescu merged commit c1ccf04 into WordPress:master Nov 22, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.