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

Media & Text block should not use background image (Closes #52789) #58514

Closed
wants to merge 2 commits into from

Conversation

nicomollet
Copy link

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 img or video tag, with object-fit: cover.

Why?

Fixes: #52789

How?

The block was updated and all background-url properties were removed.
The object-position property (defined with the focal point setting) was passed to the media.

Testing Instructions

  1. Insert a Media & Text block
  2. Add a media (image or video)
  3. Check "Crop image to fill entire column" in the block settings
  4. See that the media is now displayed with a <img> (or <video>) tag, instead of background-url.

@nicomollet nicomollet requested a review from ajitbohra as a code owner January 31, 2024 17:13
Copy link

github-actions bot commented Jan 31, 2024

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.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Block] Media & Text, First-time Contributor.

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.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 31, 2024
Copy link

👋 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.

@skorasaurus skorasaurus added the [Block] Media & Text Affects the Media & Text Block label Feb 1, 2024
Copy link
Contributor

@stokesman stokesman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +90 to +106
.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%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here width: 100% is redundant as it’s already applied by a preceding ruleset.

Comment on lines +92 to +101
border: none;
bottom: 0;
box-shadow: none;
height: 100%;
left: 0;
margin: 0;
max-height: none;
max-width: none;
object-fit: cover;
outline: none;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +104 to +105
right: 0;
top: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd opt to use inset: 0; instead of all the separate sides (including bottom and left in the preceding lines).

Comment on lines +47 to +49
focalPoint
? focalPosition(focalPoint)
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -56,6 +63,7 @@ export default function save( { attributes } ) {
className={ linkClass }
href={ href }
target={ linkTarget }
style={ { objectPosition } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s no need to add object-position to the a element.

Comment on lines +42 to 44
export function focalPosition( { x, y } = DEFAULT_FOCAL_POINT ) {
return `${ Math.round( x * 100 ) }% ${ Math.round( y * 100 ) }%`;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@skorasaurus
Copy link
Member

Hi,

Thank you for your effort on this. Unfortunately, another PR #64981 was merged into the Gutenberg plugin to close #52789, rendering your pull request unnecessary.

@skorasaurus skorasaurus closed this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media & Text block should not use background image
3 participants