-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add toggle on featured image to fallback to first image of post #65107
Conversation
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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @efu98! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Tagging in @WordPress/gutenberg-design for feedback here! I'd love to see this land. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think everything about the code is correct. There may be room for improvement regarding the label.
I agree with exposing this attribute as UI, but #56573 mentions the following:
We decided not to expose this setting in the UI for now. Since this is a more advanced feature, we are happy for now to only expose it through an attribute that theme builders can add via the code.
This is definitely a feature we want. We even had a mockup for this a while back, which I can't find, but eventually this feature grew into what is today "block connections". The idea being that virtually any block that shows an image, should be able to connect to both the basic data sources (featured image, first attachment), or indeed external sources as defined in functions.php: gb-19-block-bindings.mp4For that reason, I would think the UI we end up surfacing should be the same. Not necessarily the above shown, but perhaps an evolution of that. Mainly I'd be concerned about landing the toggle and then having to support that forever, as a separate UI to what block bindings would be able to do. I'd defer to a developer on insights here. |
Hello! Thanks for all your feedbacks! |
Thank you again for the work. Whether this PR can move forward or not is mainly dependant on having a good plan for the final look of the user interface. In this case, the toggle is just another setting, that might work. But for the final UI we need to consider all the different places you can replace media (block toolbar and inspector for cover, featured image, image, media & text, site logo), and all the different sources you can replace them with (featured image, uploaded image, image from URL, image from media library, a registered block binding such as I'd like to defer to developers like Aki who's participating in the discussion here: if we added the toggle today, would it be easy without back-compat concerns to move it to a unified UI at some point in the future? Or is it better even today, to start hooking into that "Replace" dropdown that we are unifying across media blocks? Example from the Cover block: |
My understanding is that if the feature is supported internally as an attribute, we need to continue to support it regardless of whether the UI is exposed, so adding a toggle itself doesn't concern me.
This is not difficult to achieve. We can achieve this by making the following changes in the trunk branch: Detailsdiff --git a/packages/block-library/src/post-featured-image/edit.js b/packages/block-library/src/post-featured-image/edit.js
index dff34379e4..4a340bbf6a 100644
--- a/packages/block-library/src/post-featured-image/edit.js
+++ b/packages/block-library/src/post-featured-image/edit.js
@@ -14,6 +14,7 @@ import {
PanelBody,
Placeholder,
Button,
+ MenuItem,
Spinner,
TextControl,
} from '@wordpress/components';
@@ -353,6 +354,8 @@ export default function PostFeaturedImageEdit( {
* - Has an image assigned
* - Is not inside a query loop
* Then display the image and the image replacement option.
+ * @param root0
+ * @param root0.onClose
*/
return (
<>
@@ -367,7 +370,22 @@ export default function PostFeaturedImageEdit( {
onSelect={ onSelectImage }
onError={ onUploadError }
onReset={ () => setFeaturedImage( 0 ) }
- />
+ >
+ { ( { onClose } ) => (
+ <MenuItem
+ onClick={ () => {
+ setAttributes( {
+ useFirstImageFromPost:
+ ! useFirstImageFromPost,
+ } );
+ onClose();
+ } }
+ isPressed={ useFirstImageFromPost }
+ >
+ { __( 'Fallback to first image from post' ) }
+ </MenuItem>
+ ) }
+ </MediaReplaceFlow>
</BlockControls>
) }
<figure { ...blockProps }> However, we cannot adopt this approach right now because the current Post Featured Image block does not display the Replace button unless the media is set, whereas the #64288 aims to show the add media button whether media is set or not. I think this also applies to the Post Featured Image block. So I think the path forward is either:
Also, I don't know if the UI this PR is aiming for will be supported as the Block Bindings API in the future. @SantosGuillamot and @cbravobernal may know something about this 🤔 |
Just looking now at the code, yes I guess this already exists as an attribute. That makes the back-compat conversation much simpler.
Would it be reasonable to make the same changes to the Featured Image block as we've recently made to the Image block? That is, show "Add image" or "Add media" when it's in the placeholder state? That would let us add the "Use first image attachment" option to both the Add and Replace options. |
That's right. The Post Featured Image block doesn't need a white placeholder in the first place, so the flashing that was discovered in the comment wouldn't occur. If we need this implemented before this PR, I'll be happy to submit a PR. |
I think it's a good idea to use "Add image" to show the option! Please let me know if there's anything I can do. |
It should only ever be one button, there should not be two ways to do it. But whether that's two things under the hood (the existing attribute, and the new block binding), I'll defer to developers how/whether that might be possible. |
Also, it seems that just updating the attribute is the easiest solution. Current solution is using HTML API, which is also similar to what bindings do. |
I am still not sure if the Having said that, in terms of code, given that the |
@cbravobernal @SantosGuillamot Thank you for your feedback! It's reassuring to know that adding new UI is unlikely to be an issue in the future. To move this PR forward, I'd like to work with the following suggestions:
|
I assume I am having a few missing spots of information here and there. For instance can a user add an Image block and click the drop down to select the specific Image as featured? |
Hello @paaljoachim, it seems like there are only 3 option on the Replace dropdown so there is no way to set a specific image as the featured image. |
Hey @efu98 Elise. |
What?
Add a toggle in the featured image settings to fallback to first image if no featured image is set
Why?
Display an image for old blog post that do not have featured images.
Closes #39170
How?
Toggle sets the already existing attribute useFirstImageFromPost from the featured image block. (See #56573)
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast