-
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
Post Featured Image: Add size selector #38044
Conversation
@@ -19,7 +19,8 @@ function render_block_core_post_featured_image( $attributes, $content, $block ) | |||
} | |||
$post_ID = $block->context['postId']; | |||
|
|||
$featured_image = get_the_post_thumbnail( $post_ID ); | |||
$sizeSlug = isset( $attributes['sizeSlug'] ) ? $attributes['sizeSlug'] : 'post-thumbnail'; |
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 decided to fall back to post-thumbnail
, so this change doesn't break current behavior.
const imageSizeOptions = imageSizes | ||
.filter( ( { slug } ) => { | ||
return media?.media_details?.sizes?.[ slug ]?.source_url; | ||
} ) | ||
.map( ( { name, slug } ) => ( { | ||
value: slug, | ||
label: name, | ||
} ) ); |
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 should probably memoize this value, but I'm not 100% convinced that it won't be premature optimization.
Size Change: +944 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
Very nice, thanks for working on this. Here's before: Here's after: As a small step, I'd put the size selector at the bottom, below the scale control, as the scale control is directly contextual (and appears as a result of) the width/height controls, and to reduce the prominence. A question also immediately comes to mind: if we are to allow the size selector, should we not show it regardless of whether you've added an image to the block or not? The featured image block, in my estimation, is the most useful when unset and inside a loop. If I can only set the size property when I've added an image first, the feature might not be as useful. One thing that's unique about the Size selector is that it's not really clear what it does. As a long time WordPress user, I know that it lets you choose between the different sizes that WordPress auto-bakes for you as soon as you upload an image. But for anyone not familiar, the term "image size" might suggest dimensions, and otherwise confuse in context of width and heights. Because of that, I would suggest the Size selector be a non-default field that you can explicitly add from the tools panel ellipsis menu. That might also solve the problem of it not appearing when the block doesn't have an image. What do you think? Finally, and this is not something to address in this PR, but something we should ticket as general improvements to size selectors across blocks. Firstly, changing to sentence case to match the rest of the UI: "Full size" instead of "Full Size". Secondly, I think it would be useful/helpful with showing the dimensions of the particular file right in the dropdown, like so: Would that be possible? One more thing, the alignment control in this screenshot, how can I reproduce that? I don't see it in the Latest Posts block, and we really need to wrap that control onto two lines so the "Image alignment" label itself doesn't wrap. |
Thank you for the great feedback, @jasmussen. What do you think about reverting #36540 and going with the initial design proposed in the issue? It will also match other image settings I mentioned in my previous commit.
The image size options are based on image sizes available for each media item + editor image size settings. e.g., The image with size 250x250 won't have the "Large" option available since default WP installation won't generate it. So we need selected media to generate correct size options.
Sounds good.
I think the font size selector uses something similar in design. So maybe we can adopt it.
You'll need to enable "Display featured image" for the latest post block. |
In principle I think it's okay for width, height, and scale options to be part of a generic tools panel. Can you expand on what might make that problematic?
That's a tricky piece indeed. Could we change the behavior to be aspirational? I.e. if you choose Large even when no image is loaded, what it means is that "Large" will be chosen if available, otherwise it will be the next largest source? The idea being that it'd be useful as a set it and forget option for featured images in a loop. I think the change might be important too, for future cases. I'd love to see the idea of "featured image" becoming more of a data source than a block, so that I could use a featured image inside Media & Text, for example. (#35221)
👌
Ah, thank you! Yes that's trunk, I'll see if I can push a separate fix. |
Oh I saw @jameskoster just created an excellent ticket where some of the discussion here might be relevant: #38050 |
One reason is that we can't use |
Ah, gotcha. So, keeping in mind the following doesn't necessarily have to block this PR from happening, and could be a followup anyone could work on, is there anything we can do to make it possible to add to the tools panel? It's entirely likely I'm missing some obvious reason, but it seems to me like it should be possible to add this to the tools panel, and whatever is blocking it could be worth fixing at the root. |
That's correct. We can do this in follow-up PRs.
The |
The image size control, in addition to the entire image block inspector, could use a little design love on its own, it's somewhere on my todo list! But I think we could get it to a point where adding it as one big item would be totally fine. I don't think it has to be a hard rule that each tool is a single input field. In order to move this PR forward without all that, though, I wonder if some pieces would fall in place if we moved the size selector above the width/height controls 🤔 — the key to me is that the contextually appearing scale control appears right after the controls that invoked it, i.e. width/height. |
This is looking great! Personally I still think the ToolsPanel is a good fit, in particular to @jasmussen's point about the Size selector being potentially confusing -- it seems like a good use case for a non-default control.
Linking the height & width into a single ToolsPanelItem sounds good to me as well. I wouldn't anticipate a need for keeping them separate. It's not quite the same, but there's precedent with linking border properties into a single item. |
I moved the "Sizes" panel below scales and made it a non-default control. I can do a follow-up PR for size hints in the dropdown. |
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.
It's working well :)
I'm not sure if it's something to do here or as a part of #38050, but it might be nice to add some helper text to the Size dropdown to clarify what it does. Probably doesn't need to hold this up.
Based on the discussion and some of the separate action items, I created #38068 with some initial mockups for image block inspector improvements. Please add any thoughts you have! |
Just wanted to note that inspired by this PR I've done some initial mockup work on a Latest Posts block inspector refresh. There's no task attached to it yet, and I'm not satisfied with the balance, and I don't think we are technically able to do with the dimension controls what's shown. But with that in mind, I'm sharing here just in case it inspires feedback or better ideas; this is a mockup I expect to come back to and refresh before it gets its own ticket. Before: After: |
It looks much nicer, @jasmussen 🦸 |
Perhaps something like:
|
I'm worried that this might be a little misleading for inexperienced users. The responsive images can handle loading smaller images on mobile/tablet devices in the core. |
Jeez I totally forgot about that :) So all that really matters is to choose a Size that doesn't look blurry? 🤔 |
Something like that 😄 Or if you want to fine-tune the output. Should we go with this as a help text update? - "Select the size of the source image." |
I'm not sure how helpful that really is, but probably better than nothing :D I wish there was an "auto" option for this setting that used the correct size based on height / width. |
That would be neat. |
packages/block-library/src/post-featured-image/dimension-controls.js
Outdated
Show resolved
Hide resolved
{ !! imageSizeOptions.length && ( | ||
<ToolsPanelItem | ||
hasValue={ () => !! sizeSlug && sizeSlug !== DEFAULT_SIZE } | ||
label={ __( 'Size' ) } |
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.
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 don't have any preference here, but since we're trying to unify image dimensions controls across the blocks would be nice to agree on a single ToolsPanelItem
label.
@jasmussen, @jameskoster, what do you think?
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.
In my mockup (#38068 inspired by and paired with #38050) I changed it from Image size to Size, exactly because "Image size" as a term seems to suggest dimensions (width and height). However I can see how "Size" on its own isn't necessarily better 🤔
This might be one of the situations where we keep the "Image size" label that it's had for a long time, and then seeking a better name be a part of addressing #38050.
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 have updated the label. I'm going to merge this, and we can change labels back after settings unification.
Hi, @NicoHood The control can be enabled from the Dimensions panel options. |
Oh wow, that is amazing! I feel so stupid now. Thanks a lot! I am not sure if it make sense to make this more prominent than the fixed size dimensions. The fixed size broke the layout in most cases anyways, so why not superceed this with this option and move it outside this hidden submenu? |
I can remember the exact reason, but we can always change what's displayed by default if there's a good case of it 👍 |
How does one set these Panels as “open” as default behavior. I have removed many controls but the panels that do matter for my theme setup are hidden, I rather not ask clients to go find and collapse this them selves. I didn’t read about any setting in theme.json to set which “setting” panels are open on default? How do we do this? |
You may need to allow for the control of Dimensions->Image size in the Control Panel by
in theme.json |
Description
Resolves #33789.
PR adds image size dropdown to the Featured Image block. This new option will be displayed in the "Dimensions" panel.
Now that I added yet another option to "Dimensions" control, I think they look a little out of place. Maybe we should go with the initial design proposed in the issue?
How has this been tested?
Note: Size dropdown isn't displayed if Post Featured Image has no media selected.
Screenshots
Types of changes
Enhancement
Checklist:
*.native.js
files for terms that need renaming or removal).