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.

Sign up for GitHub

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

Cover: Merge block and global styles #49434

Merged
merged 3 commits into from
Mar 30, 2023
+8 −12
Merged

Cover: Merge block and global styles #49434

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2962237
Cover: Merge block and global styles
Mamaduka Mar 29, 2023
c9a3f1f
No need to re-apply minHeight
Mamaduka Mar 29, 2023
b8278ba
Fix unit tests
Mamaduka Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 6 additions & 10 deletions packages/block-library/src/cover/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,14 @@ function render_block_core_cover( $attributes, $content ) {
}
$current_featured_image = get_the_post_thumbnail_url();

$styles = 'background-image:url(' . esc_url( $current_featured_image ) . '); ';

if ( isset( $attributes['minHeight'] ) ) {
$height_unit = empty( $attributes['minHeightUnit'] ) ? 'px' : $attributes['minHeightUnit'];
$height = " min-height:{$attributes['minHeight']}{$height_unit}";

$styles .= $height;
}
Comment on lines -52 to -57
Copy link
Member Author

Choose a reason for hiding this comment

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

The min-height is already serialized in the content. So there's no need to add it separately now that the style attribute values are merged.

P.S. We might implement this via the dimensions support flag. See #45300.


$processor = new WP_HTML_Tag_Processor( $content );
$processor->next_tag();
$processor->set_attribute( 'style', $styles );

$styles = $processor->get_attribute( 'style' );
$merged_styles = ! empty( $styles ) ? $styles . ';' : '';
$merged_styles .= 'background-image:url(' . esc_url( $current_featured_image ) . ');';
Comment on lines +53 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$styles = $processor->get_attribute( 'style' );
$merged_styles = ! empty( $styles ) ? $styles . ';' : '';
$merged_styles .= 'background-image:url(' . esc_url( $current_featured_image ) . ');';
$styles = $processor->get_attribute( 'style' );
$merged_styles = ! empty( $styles ) ? $styles . ';' : '';
if ( $current_featured_image ) {
$merged_styles .= 'background-image:url(' . esc_url( $current_featured_image ) . ');';
}

Maybe this should be done as a follow-up PR, but how about preventing the background-image property with an empty url from being output when there is no post featured image?

empty-url

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. Let's do this in a follow-up 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, there's no need for that. You can't enabled a fixed background if if the featured image isn't set. So this code is never executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the following steps could happen:

  • Add the featured image to the post from the Post panel
  • Insert a cover block
  • Enable "Fixed background" or "Repeated background"
  • Delete the featured image from the Post panel

After this procedure, the media settings will not appear in the cover block, but the hasParallax or isRepeated attributes will be retained.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Never trust the user 😄

I've changed on a local branch and will push it shortly.


$processor->set_attribute( 'style', $merged_styles );
$content = $processor->get_updated_html();
}

Expand Down