-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Start with featured image in media placeholder #41722
Conversation
Size Change: +7.09 kB (+1%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
d491ffd
to
e91f372
Compare
@@ -71,6 +71,7 @@ export function MediaPlaceholder( { | |||
onSelect, | |||
onCancel, | |||
onSelectURL, | |||
onToggleFeaturedImage, |
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'm not sure about this name. Would onSelectFeaturedImage
be better?
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 not, because we're not selecting, we're toggling the use of the featured image as a source.
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.
LGTM. One nitpick about naming. I can see an opportunity to simplify the code but I'll open an PR for that.
Here's the refactoring idea: #41742 |
This works fine, but it is strange that you can select the option if there isn't a featured image set. This is because the placeholder implies that the post contains a featured image, but that is not true. Also, when you add a cover block this way, you end up inside a paragraph block, which adds more confusion. A better approach would be only to show the button in an enabled state when the post contains a featured image. If that's not the case, we show a disabled button. This way, it will be evident to users if they have or don't have a featured image set. And in case they don't, they'll know that the cover block supports linking its media to the featured image. |
@javierarce this reasoning would also apply to the toggle in the add media dropdown in the block toolbar. In the context of a post it may be weird but in the context of template building you don't know if there is a featured image. To reconcile we have that placeholder illustration (the featured image block contains it as well). In 4bd4761 I added your suggestion, but I don't think the disabled state is helpful. In fact I would always rather to end up with a placeholder than to have a disabled button. |
e91f372
to
4bd4761
Compare
Thanks for adding my suggestion. I think this behavior makes more sense than the initial one, but let's get more eyes on this @WordPress/gutenberg-design. Something that we should address (with or without this disabled state) is the design of this placeholder, it's getting a bit messy. I'll open a new issue for this once we settle this discussion.
What do you mean by this? 🤔 |
Hah, I personally find disabled states like this frustrating because it isn't always obvious why something is disabled. If we're going to conditionally disable it I'd prefer to hide it from the UI altogether. That said, I find the placeholder to be satisfactory as a fallback, but when you elect to use the featured image (in the post editor context) shouldn't we go ahead and open the media library so that you can select/upload a featured image immediately? |
I see your point… ok, maybe hiding it completely would be a better approach.
I'm not sure what you mean by this. When would we open the media library exactly? |
When you select that you want to use the featured image (when a featured image is not already set of course). I dug up the original comment: #40156 (comment) |
Oh, that solution is nice and would reduce the confusion of the disabled/hidden states… but should we have the same text in both cases? |
Sorry, which cases? 😅 |
When there's a featured image set and when there isn't.
|
Ah I see. Yeah that could be nice. Another option would be to leave the label the same but adjust the media library modal title to say something like "Select or upload media to use as featured image". |
@javierarce @jameskoster I am unsure if this PR is "blocked" or we can merge and iterate :D The addition of the button itself is a nice quality of life addition. Do you think merging this without the deep integration to the featured media feature is a no go? |
4bd4761
to
8258f1f
Compare
Adding a featured image link to the Cover block placeholder is helpful. It would be even more helpful to be able to also set the featured image at the same time if one has not been set. |
@paaljoachim yes that is true. I am thinking about how this could work since introducing this "set featured image functionality" to the cover block would change the fact that we toggle the binding off when other media is selected. The interaction in the post featured image can't be used since the "chip" is where the cover block content is. ... still thinking 😁 |
Marked three related PRs as needing backports to 6.0.2 including this PR, #41476, and #41460. Marking them as backports because, currently, with 6.0.1, the "set as featured image" option in the toolbar leads to a confusing UX/dead end where you can't add anything further until you select either an image from the media library or a color. Here's a quick video: featured.image.movNoting this here for this specific PR since I think it does the most to resolve the current pain points :) |
I removed this PR from the WordPress 6.0.2 backporting plan. Unfortunately, these changes depend on a larger refactoring that is rather risky to bring in the bug fix release. It also doesn’t apply cleanly, so I don’t feel comfortable patching the branch for WordPress core manually. |
What?
This PR allows supporting media blocks to offer users the option to start
with featured image from the placeholder state.
Why?
To avoid having to pick another media to only then get the option and
because the featured image is now hidden behind the "Add media" block
control in the media replace flow.
How?
The PR adds a button to the media placeholder which only shows up if a
handler is provided for clicking it. It is also implemented in the cover
block because this block has a handler.
Testing Instructions
Screenshots or screencast
featured-image-placeholder.mp4