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.

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

Added toolbar dropdown action to set any image as feature image #65896

Merged
merged 18 commits into from
Nov 6, 2024

Conversation

up1512001
Copy link
Member

What?

  • if the feature image is not set for the post then if someone checked the toggle control of the image to set it as a feature image then it will be set as a feature image for the post.

Why?

fixes #65889

How?

  • added toggle control in inspector control to Set this image as feature image.
  • if isFeatureImage attribute is set & post doesn't have a feature image then it will be set as a feature image for the post.

Testing Instructions

  • create a new post.
  • add image block, and check Set this image as feature image toogle control of settings to set this image as feature image.
  • check feature image of post.

Screenshots or screencast

Screen.Recording.2024-10-06.at.16.29.51.mov

@up1512001 up1512001 added [Type] Enhancement A suggestion for improvement. [Block] Image Affects the Image Block labels Oct 6, 2024
@up1512001 up1512001 self-assigned this Oct 6, 2024
Copy link

github-actions bot commented Oct 6, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: up1512001 <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Co-authored-by: paaljoachim <[email protected]>
Co-authored-by: scruffian <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@paaljoachim
Copy link
Contributor

paaljoachim commented Oct 6, 2024

Nice job @up1512001 Utsav!

I like what I see but it needs a broader view. As there might also be some similar/associated issues that might need to be considered.

I will buzz the Gutenberg designers here: @WordPress/gutenberg-design
To get feedback on this PR.

@paaljoachim paaljoachim added the Needs Design Feedback Needs general design feedback. label Oct 6, 2024
@t-hamano
Copy link
Contributor

t-hamano commented Oct 7, 2024

Thanks for the PR!

There are a few concerns with the approach to running set_post_thumbnail() dynamically:

  • This will not work if the image source is a URL instead of media.
  • This will not work if the image is placed inside a query loop or in a single template.
  • If there are multiple images with "Set this image as feature image" enabled in the content, the first image will be set as the featured image, but the settings for the second and subsequent images will be ignored.
  • The featured image will not update until the image is actually rendered, i.e. until you save the post.

I think of this feature as an action button, not a toggle, so it should work like this:

  • The "Make featured image" button will only be displayed in the block toolbar if the block has access to the post context, i.e. inside the post editor or the query loop block.
  • When the button is clicked:
    • If the image source is media, it will actually update the featured image based on that media.
    • If the image source is a URL, it will download the image and upload it to media. Then it will actually update the featured image based on that media. TheIimage block's source will also be updated from URL to media.
  • The user will be notified via the snackbar when the featured image has been updated.

I need some design feedback, but I'm thinking of something like this:

image

@jameskoster
Copy link
Contributor

Providing a way to set the featured image via compatible image blocks seems like a good idea to explore. I agree with @t-hamano that a toggle in the Inspector doesn't quite work, and the reasoning for placing this in the block toolbar.

However the prominence in the proposed design seems a bit too much, given it would be visible on every single image block.

I'm also curious what happens after you make the image featured. Is the link between the original block and the featured image retained? IE if you subsequently change the featured image does the original image block update, or is it a one-way interaction?

@t-hamano
Copy link
Contributor

t-hamano commented Oct 7, 2024

My guess is it's one-way - we just push it from the Image block to the featured image. We could probably link it too, but that might get a bit more complicated.

However the prominence in the proposed design seems a bit too much, given it would be visible on every single image block.

Yes, I've tried to find a suitable icon to replace it, but I haven't found one yet 😅

@paaljoachim
Copy link
Contributor

Regarding icon. In the back of my mind I have been thinking about a star icon, but I am hesitant to add the icon to the Image block toolbar. It would be seen for every Image block.
What happens if one clicks another Image block star icon? Will the first Image block star icon be deactivated?

I am though wondering if we should do another approach first. I believe there is an issue for adding first image in post content as featured in the Global style setting. As that would be a way to walk around having to add an option to each Image block.

@t-hamano
Copy link
Contributor

t-hamano commented Oct 7, 2024

What happens if one clicks another Image block star icon? Will the first Image block star icon be deactivated?

If the approach is to label the Image block as a featured image, then when one Image block's icon is pressed, I think the icons of other Image blocks should be disabled.

On the other hand, if we take the approach of pushing the current image to a featured image, the icon should remain available in all image blocks since it represents an action.

I believe there is an issue for adding first image in post content as featured in the Global style setting.

Perhaps #65107? If we could move forward with #65107, would it satisfy the needs of our customers?

@t-hamano
Copy link
Contributor

t-hamano commented Oct 7, 2024

If we were to use an icon, I would imagine something like this (sorry for the crappy icon!):

image

@jameskoster
Copy link
Contributor

I'm wondering if even the icon is too prominent. While handy, this doesn't feel like an affordance users will use all that often? Would it make sense in the ellipsis menu?

@t-hamano
Copy link
Contributor

t-hamano commented Oct 7, 2024

Would it make sense in the ellipsis menu?

This is possible. It looks like there are two slots in the ellipsis menu, and we can inject a button at the top, or between "Create pattern" and "Edit as HTML":

Header Header
image image

@up1512001
Copy link
Member Author

@t-hamano @jameskoster I think if we add this kind of toolbar control it will be much better. let me know your view.

Screen.Recording.2024-10-08.at.00.26.19.mov

@jameskoster
Copy link
Contributor

@up1512001 Generally speaking the toolbar is a location reserved for objectively the most common actions users might apply. Ideally they are carefully curated to avoid an overwhelming/confusing number of options.

Over the course of a post or pages lifetime how often do we expect users to make use of this feature? In most cases it will be once, and that's assuming they don't set the featured image via the dedicated control. For that reason, if we add this feature I'd prefer to see it in the ellipsis menu. Another benefit is that the action becomes available in List view.

@t-hamano of those options I think the second one makes most sense.

@t-hamano
Copy link
Contributor

t-hamano commented Oct 8, 2024

@up1512001 We should be able to inject buttons into the ellipsis menu via the BlockSettingsMenuControls slot. See the implementation in the Template Part block for reference.

Comment on lines 1072 to 1079
<ToolbarButton
icon="format-image"
label={ __( 'Make Feature Image' ) }
onClick={ () =>
setAttributeForFeatureImage( isFeatureImage )
}
isPressed={ isFeatureImage }
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

@paaljoachim @jameskoster @scruffian

I want to confirm again here: does the "Make Feature Image" button mean to label the block to be used as a featured image? Or is it meant to update the featured image with the current image?

For example, let's assume that the former (labeling) is what is expected here. Furthermore, suppose that useFirstImageFromPost is enabled for the featured image and a second Image block in the content is labeled to be used as a featured image.

In this case, which one will actually be displayed as the featured image?

To prevent such inconsistencies, I prefer the latter approach (updating the featured image with the current image).

Copy link
Contributor

Choose a reason for hiding this comment

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

The latter was my expectation. But I don't have a strong feeling about this feature and would welcome more feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect to use the image as a selected featured image.

That is when I press the link/button "Set featured image" associated with the Image block a featured image should also be added here:
Screenshot 2024-10-08 at 20 34 13

It becomes an additional option to directly set the Featured image through the ellipse (3 button drop down) in an Image block. As it is added here it could easily also be added to a Cover block, Media & Text block etc.
If there was already a featured image added to the post then clicking the option in the ellipse would then updated the featured image with the one that was just selected through the Image block.

Conclusion is that having an additional method to add a featured image would be helpful.

--

Now on another similar issue.
It would also be helpful to set first image in post as featured image through Global settings. This means if one forgets to set a featured image then the first image in the post will automatically be set as featured. I have experienced over and over that customers forget to add a featured image. Having this happen automatically would very much help.

@paaljoachim
Copy link
Contributor

It looks good @up1512001

You can use the same language "Set featured image" as is used in the post settings area for the featured image.

@t-hamano
Copy link
Contributor

t-hamano commented Oct 9, 2024

@up1512001 Thanks for the update!

We need to improve the code and address more scenarios to move this PR forward.

Here's what I can think of for now:

  • The block attribute, i.e. isFeaturedImage, should no longer be necessary.
  • This function should not be available if we can't reference the post id or if we're in a query loop. For example, the Featured Image has a check for those. This function should have a similar check.
  • We should use useEntityProp instead of dispatch. Additionally, using the core/editor store is not recommended (see this example). Here's a good reference on how to update the featured image via useEntityProp.
  • The image source may be a URL instead of a media. If the image source is a URL, this button will not work. If we also want to consider the URL source, we need to take care of uploading the image to the media beforehand. Personally, I think this PR should only consider the media source, and address the URL source in a follow-up.

If you have any questions, please feel free to contact us 👍

@up1512001
Copy link
Member Author

@t-hamano Addressed your feedback please review this PR.

  • if an image block is added into the query loop then the Set featured image control will not be available.
  • Also if the current image is set as a feature image then this control won't be visible.
Screen.Recording.2024-10-24.at.23.47.57.mov

cc: @paaljoachim

@up1512001 up1512001 requested a review from t-hamano October 24, 2024 18:44
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the update! This PR is working fine. It even works in the Pages screen of the site editor:

4e187ea185f0fbb59670a80cc1b9fa05.mp4

As suggested in the comment, this doesn't work if the image source is a URL. I think we need to check attributes.id for now.

packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
@up1512001
Copy link
Member Author

@t-hamano updated as per your suggestions 🙇‍♂️

  • Added attributes.id check if image source is URL.
  • added snackbar success message.
  • removed check that was hiding the Set featured image option if the current image is the featured image.
Screen.Recording.2024-11-04.at.19.42.12.mov

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the update! From a code perspective, it looks almost ready to ship.

@jameskoster @paaljoachim Any feedback on the copy of the notice below?

f64055e3940f0ae33c64b4dff0b7fb57

packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
@paaljoachim
Copy link
Contributor

I got to say that this is a really nice job @up1512001 Utsav! Very nice!

The notification message looks good. I do not know if it is a similar syntex (kind) as used with other notifications so we will see what Jay says.

@t-hamano t-hamano self-requested a review November 6, 2024 07:51
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Ok, let's ship this PR. As for the notification message, if there is a more reasonable one we can address that in a follow-up.

@t-hamano
Copy link
Contributor

t-hamano commented Nov 6, 2024

@up1512001 Could you merge the trunk branch into this PR again? That should solve the stalled CI (See #66771).

@up1512001
Copy link
Member Author

@up1512001 Could you merge the trunk branch into this PR again? That should solve the stalled CI (See #66771).

@t-hamano latest trunk is merged.

@up1512001 up1512001 merged commit 84048f8 into WordPress:trunk Nov 6, 2024
61 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Nov 6, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…image is set for post (WordPress#65896)

* feature: created toggle control to set content image as feature image if no feature image is set

* Fix: docs build

* revert: php changes for feature image

* feature: created toolbar control to set image as feature image is post don't have feature image set

* feature: created block settings & added proper notices for success message

* update: added required block context for feature image control

* update: set feature image control as per suggestion

* remove: unnessary isFeature image attribute

* rename: feature image control

* Fix: query loop issue

* Fix: typo in set featured image

* update as per suggestions

* Fix: minor feedback

* Fix: minor issue
@priethor priethor changed the title Added toggle control to set any image as feature image if no feature image is set for post Added toolbar dropdown action to set any image as feature image Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image Block: Add "make featured image" option
4 participants