Makes cover block dynamic and adds featured image binding #39658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
draganescu
requested review from
TimothyBJacobs,
spacedmonkey,
ajitbohra,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
draganescu
added
[Type] Enhancement
|
Size Change: +430 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
getdave
reviewed
Mar 23, 2022
There was a problem hiding this comment.
Firstly - thank you so much for working on this feature. It's important and will be greatly appreciated by lots of people 👏
I tested against your instructions and it worked 🎉
Screen.Capture.on.2022-03-23.at.10-43-21.mov
I then proceeded to stress test and I think I've found a bug.
If you copy a block which is using the featured image into a new post which does not have a featured image, the image does not update to reflect this fact. This can lead to inconsistency between editor and front of site and missing images on the front of the site:
Screen.Capture.on.2022-03-23.at.10-56-38.mov
The good news is that if the post does already have a featured image then it works perfectly:
Screen.Capture.on.2022-03-23.at.11-01-22.mp4
getdave
reviewed
Mar 23, 2022
|
Thanks for working on this. We're getting close! Here are some issues I found:
|
getdave
reviewed
Mar 23, 2022
draganescu
force-pushed
the
add/cover-featured-image-background
branch
2 times, most recently
from
a270352 to
b18487d
Compare
|
This is ready for a new review. It now:
|
|
This is looking great. A few things I found:
|
scruffian
reviewed
Mar 25, 2022
scruffian
reviewed
Mar 25, 2022
scruffian
reviewed
Mar 25, 2022
scruffian
reviewed
Mar 25, 2022
I would not want to implement a placeholder as that would also need to implement setting a featured image from the cover block. What other behavior would be good I wonder? Or maybe "just" a placeholder, w/o the upload thing on top, that could work.
I think this is a feature 😆 , the block retains the image but loses the behavior. Is that so unexpected?
|
draganescu
force-pushed
the
add/cover-featured-image-background
branch
from
ce012b3 to
dd6e0eb
Compare
I'm not sure what the best placeholder should be, but if I set the cover to use featured image, and there isn't one set then I would not expect to see the original image - I'd expect to see something that shows me that I haven't set a featured image - otherwise the function of the button isn't clear.
Yeah it is unexpected because the frontend doesn't match the backend. If the featured image is removed then we should remove it from the block as well. |
|
This is looking great. I retested and everything works as I expected. I did find a strange bug with clearing media: coverclear.mp4I don't think this needs to be addressed before we merge though. |
scruffian
reviewed
Mar 28, 2022
scruffian
approved these changes
Mar 28, 2022
youknowriad
reviewed
Mar 28, 2022
youknowriad
reviewed
Mar 28, 2022
youknowriad
reviewed
Mar 28, 2022
This was referenced Apr 13, 2022
|
When I set the featured image cover block in a template file, the featured image doesn't show. If then I trigger a save (without any edit) from the site editor, overwriting the template with the same code, everything works as expected. wp-cover-featured.mp4[Edit: since for some reasons the first part of the above embedded video is suffering from the same problem of the featured image, this is the url: https://user-images.githubusercontent.com/6989338/166814081-c1c88127-1605-406c-8b16-093e4add151c.mp4] The template file I'm using in the video, in a Twenty Twentytwo child theme, is <!-- wp:cover {"useFeaturedImage":true,"dimRatio":50,"overlayColor":"foreground","minHeight":80,"minHeightUnit":"vh","contentPosition":"center left","style":{"spacing":{"padding":{"top":"min(4vw, 90px)","right":"min(4vw, 90px)","bottom":"min(4vw, 90px)","left":"min(4vw, 90px)"}}}} -->(I've also tried with an Archeo child theme...same behavior) |
|
I think the issue here is that these two themes use a solid color for the overlay in the cover block, so the featured image is there, but its hidden behind the overlay. You can see this in Archeo with the |
|
@scruffian I checked the generated html. The image is not there. Edit: <div class="wp-container-5 wp-block-column is-vertically-aligned-bottom"
style="padding-top:0px;padding-right:0px;padding-bottom:0px;padding-left:0px">
<div class="wp-block-cover has-custom-content-position is-position-center-left"
style="padding-top:min(4vw, 90px);padding-right:min(4vw, 90px);padding-bottom:min(4vw, 90px);padding-left:min(4vw, 90px);min-height:80vh">
<span aria-hidden="true"
class="wp-block-cover__background has-foreground-background-color has-background-dim"></span>
<div class="wp-block-cover__inner-container">
<div style="text-transform:uppercase;" class="wp-block-post-date"><time
datetime="2022-05-04T13:32:49+00:00">May 4, 2022</time></div>
<h1 style="text-transform:uppercase;" class="wp-block-post-title has-huge-font-size">Private: Hello</h1>
</div>
</div>
</div><div class="wp-container-5 wp-block-column is-vertically-aligned-bottom"
style="padding-top:0px;padding-right:0px;padding-bottom:0px;padding-left:0px">
<div class="wp-block-cover has-custom-content-position is-position-center-left"
style="padding-top:min(4vw, 90px);padding-right:min(4vw, 90px);padding-bottom:min(4vw, 90px);padding-left:min(4vw, 90px);min-height:80vh">
<span aria-hidden="true"
class="wp-block-cover__background has-foreground-background-color has-background-dim"></span><img
class="wp-block-cover__image-background" alt=""
src="https://example.com/wp-content/uploads/2022/05/beach-resort-1395730.jpg"
style="object-position: " data-object-fit="cover" data-object-position="">
<div class="wp-block-cover__inner-container">
<div style="text-transform:uppercase;" class="wp-block-post-date"><time
datetime="2022-05-04T13:32:49+00:00">May 4, 2022</time></div>
<h1 style="text-transform:uppercase;" class="wp-block-post-title has-huge-font-size">Private: Hello</h1>
</div>
</div>
</div> |
|
Sorry if I am late to this. In I believe we need to ensure that this is done here as well. CC @draganescu |
|
@navigatrum what is the code you are using in your template file? |
<!-- wp:template-part {"slug":"header","tagName":"header","theme":"tt2-child"} /-->
<!-- wp:group {"tagName":"main"} -->
<main class="wp-block-group">
<!-- wp:columns {"align":"full","style":{"spacing":{"padding":{"top":"0px","right":"0px","bottom":"0px","left":"0px"}}},"backgroundColor":"foreground","textColor":"background"} -->
<div class="wp-block-columns alignfull has-background-color has-foreground-background-color has-text-color has-background"
style="padding-top:0px;padding-right:0px;padding-bottom:0px;padding-left:0px">
<!-- wp:column {"verticalAlignment":"bottom","style":{"spacing":{"padding":{"top":"0px","right":"0px","bottom":"0px","left":"0px"}}}} -->
<div class="wp-block-column is-vertically-aligned-bottom"
style="padding-top:0px;padding-right:0px;padding-bottom:0px;padding-left:0px">
<!-- wp:cover {"useFeaturedImage":true,"dimRatio":50,"overlayColor":"foreground","minHeight":80,"minHeightUnit":"vh","contentPosition":"center left","style":{"spacing":{"padding":{"top":"min(4vw, 90px)","right":"min(4vw, 90px)","bottom":"min(4vw, 90px)","left":"min(4vw, 90px)"}}}} -->
<div class="wp-block-cover has-custom-content-position is-position-center-left"
style="padding-top:min(4vw, 90px);padding-right:min(4vw, 90px);padding-bottom:min(4vw, 90px);padding-left:min(4vw, 90px);min-height:80vh">
<span aria-hidden="true"
class="wp-block-cover__background has-foreground-background-color has-background-dim"></span>
<div class="wp-block-cover__inner-container">
<!-- wp:post-date {"style":{"typography":{"textTransform":"uppercase"}}} /-->
<!-- wp:post-title {"level":1,"style":{"typography":{"textTransform":"uppercase"}},"fontSize":"huge"} /-->
</div>
</div>
<!-- /wp:cover -->
</div>
<!-- /wp:column -->
</div>
<!-- /wp:columns -->
<!-- wp:spacer {"height":"48px"} -->
<div style="height:48px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:post-content {"layout":{"inherit":true}} /-->
<!-- wp:spacer {"height":"32px"} -->
<div style="height:32px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:group {"align":"full","style":{"elements":{"link":{"color":{"text":"var:preset|color|background"}}}},"backgroundColor":"foreground","textColor":"background","layout":{"inherit":true}} -->
<div
class="wp-block-group alignfull has-background-color has-foreground-background-color has-text-color has-background has-link-color">
<!-- wp:spacer -->
<div style="height:100px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:group {"style":{"spacing":{"blockGap":"4px"}},"layout":{"type":"flex","allowOrientation":false}} -->
<div class="wp-block-group">
<!-- wp:paragraph {"style":{"typography":{"textTransform":"uppercase"}},"fontSize":"small"} -->
<p class="has-small-font-size" style="text-transform:uppercase">Categories:</p>
<!-- /wp:paragraph -->
<!-- wp:post-terms {"term":"category","style":{"typography":{"textTransform":"uppercase"}},"fontSize":"small"} /-->
<!-- wp:paragraph {"style":{"typography":{"textTransform":"uppercase"}},"fontSize":"small"} -->
<p class="has-small-font-size" style="text-transform:uppercase">· Tagged:</p>
<!-- /wp:paragraph -->
<!-- wp:post-terms {"term":"post_tag","style":{"typography":{"textTransform":"uppercase"}},"fontSize":"small"} /-->
</div>
<!-- /wp:group -->
<!-- wp:spacer {"height":"48px"} -->
<div style="height:48px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:columns {"isStackedOnMobile":false,"align":"wide","style":{"spacing":{"padding":{"top":"0px","right":"0px","bottom":"0px","left":"0px"}}}} -->
<div class="wp-block-columns alignwide is-not-stacked-on-mobile"
style="padding-top:0px;padding-right:0px;padding-bottom:0px;padding-left:0px">
<!-- wp:column -->
<div class="wp-block-column">
<!-- wp:separator {"opacity":"css","className":"is-style-wide"} -->
<hr class="wp-block-separator has-css-opacity is-style-wide" />
<!-- /wp:separator -->
<!-- wp:post-navigation-link {"type":"previous","label":"Previous Post"} /-->
</div>
<!-- /wp:column -->
<!-- wp:column -->
<div class="wp-block-column">
<!-- wp:separator {"opacity":"css","className":"is-style-wide"} -->
<hr class="wp-block-separator has-css-opacity is-style-wide" />
<!-- /wp:separator -->
<!-- wp:post-navigation-link {"textAlign":"right","label":"Next Post"} /-->
</div>
<!-- /wp:column -->
</div>
<!-- /wp:columns -->
<!-- wp:spacer {"height":"48px"} -->
<div style="height:48px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:post-comments /-->
<!-- wp:spacer -->
<div style="height:100px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
</div>
<!-- /wp:group -->
</main>
<!-- /wp:group -->
<!-- wp:template-part {"slug":"footer","theme":"coccalab-theme","tagName":"footer"} /--> |
|
Ah I found the problem. In the server side render we do this: If you change the structure of the markup (for example moving the div to a new line) then this no longer works. @draganescu should we strip the white space before we do the replace? |
👍 $content = str_replace(
'</span><div',
'</span>' . $image . '<div',
preg_replace( '/>(\s)+</m', '><', $content )
);would solve the problem |
|
@navigatrum do you want to open a PR? |
navigatrum
added a commit
to navigatrum/gutenberg
that referenced
this pull request
May 5, 2022
Follows on from [WordPress#39658]( WordPress#39658) Remove whitespaces from html tags generated by `render_block_core_covermarkup` before injecting the featured image
|
Testing Exploring.... Adding the Cover block and clicking Use featured image. If I have not added a featured image to the post then the Cover block shows a blank area.
I am thinking that this feature can work both ways. Click the icon to add the current Cover block image as a featured image if there is no featured image already added to the post. If the user has already added a featured image then that will be added to the Cover block. |
|
Yes @paaljoachim this is a good way forward. Perhaps a new issue to easily track progress, if one does not already exist, could work? 🙏🏻 |
|
Was this tested with #40853 @paaljoachim |
|
Is there supposed to be a UI for Also, the Playing around with this, I may be wrong, assuming it does any detection of image colors, actually. Turns out when I select black color for the overlay the |
38 tasks
adamziel
added a commit
that referenced
this pull request
Jun 27, 2022
Stopgap solution to remove whitespaces from html tags generated by render_block_core_covermarkup before injecting the featured image. Follows on from [#39658]( #39658). Remove whitespaces from html tags generated by `render_block_core_covermarkup` before injecting the featured image. For the block layout see: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/cover/edit/index.js#L327-L367 For the HTML tags capturing by class name see: https://github.com/WordPress/gutenberg/blob/51db4bf888e6b18cf9d18266108114d61070f3ad/lib/block-supports/layout.php#L232 Co-authored-by: Adam Zielinski <adam@adamziel.com>
adamziel
added a commit
that referenced
this pull request
Jun 30, 2022
Stopgap solution to remove whitespaces from html tags generated by render_block_core_covermarkup before injecting the featured image. Follows on from [#39658]( #39658). Remove whitespaces from html tags generated by `render_block_core_covermarkup` before injecting the featured image. For the block layout see: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/cover/edit/index.js#L327-L367 For the HTML tags capturing by class name see: https://github.com/WordPress/gutenberg/blob/51db4bf888e6b18cf9d18266108114d61070f3ad/lib/block-supports/layout.php#L232 Co-authored-by: Adam Zielinski <adam@adamziel.com>
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Closes #13795 and advances #37753.
Alternative to #39410.
Why?
This approach doesn't care about keeping cover agnostic from WordPress and copies the method used by the
FeaturedImageblock to bind the block's background to the featured image.How?
We get the featured image from the nearest entity. If the block is not in a post, or does not get the postId from context, the binding (including UI) is unavailable. Once the binding is active - via setting a new attribute
useFeaturedImage- then we watch for updates outside of the block to the post featured image.To handle the fact that the featured image can be changed else where we changed the
savefunction of the cover block to replace the url on the fly with a tokenWordPress://featured-imagewhich gets replaced with the live featured image by the new cover block's server side renderer.ThisReplaced with server sideWordPress://featured-imageis not a perfect solution, the other option would be, but not sure if possible, to not run any save (set save tonull) and reimplement the rendering on the server side.preg_replaceinjection of HTML.Testing Instructions
= the featured image should appear instead
= the cover block's background should change
= verify that on the front end the cover block renders with the image you've set.
Screenshots or screencast