Media & Text block should not use background image (Closes #52789) #58514
Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @nicomollet! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
There was a problem hiding this comment.
Thanks for working on this @nicomollet. It’s worked well in my testing so far but it does seem like it needs to include a block deprecation. I've yet to test that.
The biggest question I'm having at the moment is how the new styles make it possible that video can also use the "Crop image to fill entire column" setting yet you didn't make the changes to enable that in the UI. It seems like we should unless I'm missing something. Obviously that label would want to be updated so it’s not specific to images.
Other than that I've left some code feedback that I hope you'll find constructive.
| .wp-block-media-text.is-image-fill .wp-block-media-text__media img, | ||
| .wp-block-media-text.is-image-fill .wp-block-media-text__media video { | ||
| border: none; | ||
| bottom: 0; | ||
| box-shadow: none; | ||
| height: 100%; | ||
| left: 0; | ||
| margin: 0; | ||
| max-height: none; | ||
| max-width: none; | ||
| object-fit: cover; | ||
| outline: none; | ||
| padding: 0; | ||
| margin: -1px; | ||
| overflow: hidden; | ||
| clip: rect(0, 0, 0, 0); | ||
| border: 0; | ||
| position: absolute; | ||
| right: 0; | ||
| top: 0; | ||
| width: 100%; |
There was a problem hiding this comment.
Here width: 100% is redundant as it’s already applied by a preceding ruleset.
| border: none; | ||
| bottom: 0; | ||
| box-shadow: none; | ||
| height: 100%; | ||
| left: 0; | ||
| margin: 0; | ||
| max-height: none; | ||
| max-width: none; | ||
| object-fit: cover; | ||
| outline: none; |
There was a problem hiding this comment.
I'm wary of adding more styles here than strictly necessary. In particular border, box-shadow, margin, and outline. I tested without these and it was fine but I only tested with twentytwentyfour. If you can share some reasoning on why these should be added it would help me.
| right: 0; | ||
| top: 0; |
There was a problem hiding this comment.
Nit: I'd opt to use inset: 0; instead of all the separate sides (including bottom and left in the preceding lines).
| focalPoint | ||
| ? focalPosition(focalPoint) | ||
| : undefined; |
There was a problem hiding this comment.
It appears that previously even if focalPoint wasn’t set a style would be generated for the default 50% 50% and now this omits a style in such cases. I like this because object-position’s initial value is 50% 50% so there’s no need to set it. Since the save output is changing anyway I think this should be fine but just wanted to note it.
| className={ linkClass } | ||
| href={ href } | ||
| target={ linkTarget } | ||
| style={ { objectPosition } } |
There was a problem hiding this comment.
There’s no need to add object-position to the a element.
| export function focalPosition( { x, y } = DEFAULT_FOCAL_POINT ) { | ||
| return `${ Math.round( x * 100 ) }% ${ Math.round( y * 100 ) }%`; | ||
| } |
There was a problem hiding this comment.
Having both imageFillStyles and this new function seems like clutter. Previously imageFillStyles was used in a two places and still could be. Or alternatively, focalPosition could replace it in both those places. I’d prefer using one or the other and not both.
What?
In the Media & Text block, when media have "Crop image to fill entire column" enabled, the media is in background.
It can be better, such as Cover block, and be a real
imgorvideotag, withobject-fit: cover.Why?
Fixes: #52789
How?
The block was updated and all
background-urlproperties were removed.The
object-positionproperty (defined with the focal point setting) was passed to the media.Testing Instructions
<img>(or<video>) tag, instead of background-url.