Add featured image to Media & Text block #51491
Merged
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
carolinan
added
[Block] Media & Text
|
Size Change: +503 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 96c3a18. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7898039463
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
👋🏻 @carolinan 👀 looking at this |
This comment was marked as outdated.
This comment was marked as outdated.
draganescu
reviewed
Jun 16, 2023
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
How can I make this block into a dynamic block and still keep the existing anchor block support? |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/blocks.php |
|
@carolinan the anchor support is not available for dynamic blocks. Unfortunately in the rendered the value of the anchor support is unavailable - as far as I've seen - therefore we need to change how the rendered builds the markup: instead of building all the markup, leave what comes from the save function and only replace the image with the featured image (see the cover block for example). |
|
@draganescu Thank you, I think I understand. I have got it mostly working now; |
carolinan
force-pushed
the
add/media-text-featured-image
branch
from
c518c80
to
efe0241
Compare
carolinan
requested review from
spacedmonkey,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
carolinan
changed the title
29 tasks
draganescu
reviewed
Sep 26, 2023
draganescu
reviewed
Sep 26, 2023
draganescu
reviewed
Sep 26, 2023
There was a problem hiding this comment.
Good work here 🙇🏻 This tests well, found this styling issue for featured image placeholder state.
Possibly a more generous min height may work?
Left a couple of comments:
- why do we need a deprecation/migration or anything?
- a code nit, that doesn't seem to produce a bug but is susceptible to do so in the future.
This comment was marked as outdated.
This comment was marked as outdated.
t-hamano
reviewed
Jan 26, 2024
| const toggleUseFeaturedImage = () => { | ||
| setAttributes( { | ||
| imageFill: false, | ||
| mediaType: 'image', |
There was a problem hiding this comment.
If we set mediaType, an img element with no attributes will be saved. This markup is invalid because the img element requires at least a src attribute.
However, I do know that the img tag needs to be present in the markup in order to dynamically assign the src attribute during dynamic rendering. I am wondering if there is any good approach to solve this problem.
There was a problem hiding this comment.
By setting mediaType to undefined, we will be able to remove the img tag that does not have attributes from the saved markup. Instead, I believe that it's possible to inject an img tag in the dynamic rendering when useFeaturedImage is true.
We should be able to use preg_replace to inject the img element as a string right after the opening tag of the figure element. This approach is also used in the Image block. In the future, if the HTML API becomes able to inject arbitrary elements, it will be possible to refactor it at that time.
There was a problem hiding this comment.
Thank you.
I'm trying, but I can't get it to work, the image tag is only rended on the front. I must be missing a step, or there is something that we did not consider.
I will push the changes though so they can be tested.
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
draganescu
reviewed
Feb 6, 2024
There was a problem hiding this comment.
@carolinan @t-hamano instead of setting the mediaType to undefined which would need the MediaContainer to be updated, and also seems wrong because media type for featured image is image, we could avoid saving the image in the save function if useFeaturedImage is true:
- so keep
mediaType: 'image'in edit - update save to:
let image = ! useFeaturedImage ? (
<img
src={ mediaUrl }
alt={ mediaAlt }
className={ imageClasses || null }
/>
) : null;
what do you think?
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I tried it with the save function update. It is an improvement because the empty img tag is not present if the featured image option is set when there is no featured image on the post. But in this scenario, the
|
This approach seems better. If
This would be an unexpected scenario. We may need to adjust the conditional statement in the save.js function. |
|
Based on a recent Editor Triage async session - I'm moving this off the WordPress 6.5 Editor Tasks board. |
…t/media-container.js
Only output the img tag if there is a mediaURL. The mediaURL is required because img tags without a src attribute is invalid HTML.
|
I changed the condition in save.js from checking if a featured image is used, to checking if there is a |
|
I think this is a good state, any improvement can come in from of a follow up. Congratulations everyone for the tweaking and effort to see this through. Also I forgot to paste the list of contributors |
|
Thank you. |
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?
Add an option to use the featured image in a Media & Text block.
Closes #35221
Why?
The Media & Text block has a unique feature called "stack on mobile" which is useful when you want to display the post title and featured image together but with different layouts on desktop and mobile.
This was a feature requested for the "Blue Note" community theme.
How?
The featured image can be selected in the initial block placeholder and from the block toolbar.
A placeholder image is used if the featured image option is chosen, but the post has no featured image.
Testing Instructions
Test the following:
Replacing an existing image or video with a featured image
Adding a featured image when there is no featured image on the post or page
Adding a featured image when there is a featured image on the post or page
Removing the featured image from the post/page sidebar
Replacing the featured image from the post/page sidebar
Replacing the featured image from the block toolbar: by uploading a new media and by selecting from the media library
Next, please confirm that:
Testing Instructions for Keyboard
Screenshots or screencast
In this video, I start with a post that has a featured image. I add a new media & text block, enable the featured image option, and try some block toolbar settings. Then I replace the featured image with an image from the media library and then replace that image with the featured image again.
add-replace.mp4
In this video, I am enabling the featured image option even though the post has no featured image, showing the placeholder:
placeholder.mp4
In this video, I have added the featured image and an anchor to the block, and I am checking that the anchor is added on the front.
media-text-anchor.mp4
Twenty Twenty Three blog / Home template with featured image and post title in the media & text block inside the query:
query.mp4