WordPress
Image: Adds the block controls for uploading image.
What?
Fixes #54867
Why?
In small width size the image block placeholder looks broken and it's hard to choose an image from there.
How?
This PR is adding a BlockControl to choose an image which will have all the three options upload, choose from media library or add from a link and will keep the placeholder abstract.
Testing Instructions
- Add a columns block or grid block. Choose 6+ columns or grid in a row and add an image block.
- Now the choose an image option will appear in BlockControls Toolbar.
Screenshots or screencast
For Large Size
For Medium Size
For small Size
Nice! This makes good progress, but doesn't yet fully fix 54867. Quick GIF:
We need only two changes:
- Rename "Choose Image" to "Add image" (note the lowercase i)
- Don't show the "white material" in mobile.
2 should be possible with CSS. Essentially it should look like this:
That maintains the gray placeholder material rather than the white <Placeholder /> material on-select. You should be able to use the CSS class already present, for this. Something like:
- Change
.wp-block-image.wp-block-image.is-selected .block-editor-media-placeholderto.wp-block-image.wp-block-image.is-selected .block-editor-media-placeholder:not(.is-small) - Change
.wp-block-image.wp-block-image.is-selected .block-editor-media-placeholder .components-placeholder__illustrationto .wp-block-image.wp-block-image.is-selected .block-editor-media-placeholder .components-placeholder__illustration:not(.is-small)` - Change
.wp-block-image.wp-block-image.is-selected .block-editor-media-placeholder:beforeto.wp-block-image.wp-block-image.is-selected .block-editor-media-placeholder:not(.is-small):before - Change
.is-selected>.components-placeholder.has-illustration .components-placeholder__labelto.is-selected>.components-placeholder.has-illustration:not(.is-small) .components-placeholder__label
There are probably better places to do this, this was some quick inspector work. Let me know if you'd like me to try and push some code.
@jasmussen Thanks for the feedback! I'll work on these changes, or if you'd like to make any updates yourself, feel free to do so 😄. Currently, I'm trying to find a workaround for the grid layout, as the current implementation isn't working with grids due to the lack of inner blocks for the columns. I'm experimenting with the number of columns, the minimum width of columns, etc. If you have any better solutions specifically for grids, please let me know.
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: vipul0425 <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: richtabor <[email protected]>
Co-authored-by: annezazu <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Nice. Took the latest for a spin:
In the above I added a min-height: 60px; to the placeholder component. The default state is a bit small:
Here's 60px again:
CC: @WordPress/gutenberg-design in case you have opinions on the min-height of this state.
I noticed one glitch. when you insert the image in a narrow column, there's a brief flash of the white placeholder material:
It's a very brief flash, but enough that it's annoying. Since the CSS would never show it, I wonder if it's the resize-observe applying the is-small CSS class with a slight delay? Is there any trick we can do here?
Separately, I noticed that this PR adds a resizeobserver locally to the Image block. That may be fine (perhaps @WordPress/gutenberg-components would know whether this is best done directly in the Placeholder component or if it's good to keep here). I'm asking mainly because the Placeholder component already has one, and it already applies a CSS class. Could we rely on that existing CSS class? It's fine that every placeholder gets the is-small changes you've made here, even if only the Image block makes use of them for now. Make sense?
Hi @jasmussen, I have removed the ResizeObserver from this component and refactored the logic using CSS. I also added a min-height for the small variant.
@t-hamano, I have implemented the lockUrlControls logic in the external MediaReplaceFlow, and now we can also replace images. However, my only concern is that in the small size, the "Connected to post meta" text (lockUrlControlsMessage) is also hidden. Do you have any suggestions on where we can display this? I think there should be some visual distinction apart from just the purple image icon that is visible in the toolbar for this.
However, my only concern is that in the small size, the "Connected to post meta" text (lockUrlControlsMessage) is also hidden. Do you have any suggestions on where we can display this? I think there should be some visual distinction apart from just the purple image icon that is visible in the toolbar for this.
We should show this in the inspector, if it isn't already. If it's already shown there, it's okay to not show it here, and just show the same gray placeholder material.
Taking a look at the branch again now, thanks for your continued work!
Works well!
I'll defer to others for a code review. I also imagine that the brief flash of the white material is hard to fully get rid of. I wonder if—and this can be a separate followup PR—the logic should be inversed: instead of hiding the elaborate white placeholder states in narrow contexts, we show them in the wider contexts? Perhaps the flash would then also be inverted, so this might not be the best solution. But something to potentially try!
@vipul0425 Thanks for the update!
I have implemented the
lockUrlControlslogic in the externalMediaReplaceFlow, and now we can also replace images.
Sorry if my suggestion was unclear.
What I meant is, I don't think we need to add another MediaReplaceFlow component. I mean, I think we can update here and add a prop like name={ ! url ? __( 'Add image' ) : __( 'Replace' ) }. Such an implementation is also done here.
@t-hamano I tried to integrate that code here, but since it is defined within the Image component, which isn't rendered on the first load, the inspector controls aren't visible. Alternatively, we could define MediaReplace at the root level and reuse it in both the cases. Could you please guide me on whether that approach would work?
@vipul0425
How about making the following changes to this PR?
- Extract the
MediaReplaceFlowcomponent from thecontrolcomponent asmediaReplaceFlow - Make sure
mediaReplaceFlowis always rendered
Diff
diff --git a/packages/block-library/src/image/edit.js b/packages/block-library/src/image/edit.js
index c2b1755670..e867461f00 100644
--- a/packages/block-library/src/image/edit.js
+++ b/packages/block-library/src/image/edit.js
@@ -375,34 +375,6 @@ export function ImageEdit( {
return (
<>
<figure { ...blockProps }>
- { ! ( temporaryURL || url ) && ! lockUrlControls && (
- <BlockControls group="other">
- <MediaReplaceFlow
- mediaId={ id }
- mediaURL={ url }
- allowedTypes={ ALLOWED_MEDIA_TYPES }
- accept="image/*"
- name={ __( 'Add image' ) }
- onSelect={ onSelectImage }
- onError={ onUploadError }
- >
- <form className="block-editor-media-flow__url-input has-siblings">
- <span className="block-editor-media-replace-flow__image-url-label">
- { __( 'Insert from URL:' ) }
- </span>
-
- <LinkControl
- value={ { url } }
- settings={ [] }
- showSuggestions={ false }
- onChange={ ( value ) => {
- onSelectURL( value.url );
- } }
- />
- </form>
- </MediaReplaceFlow>
- </BlockControls>
- ) }
<Image
temporaryURL={ temporaryURL }
attributes={ attributes }
diff --git a/packages/block-library/src/image/image.js b/packages/block-library/src/image/image.js
index 7dd03a7fb5..775482f0ab 100644
--- a/packages/block-library/src/image/image.js
+++ b/packages/block-library/src/image/image.js
@@ -551,6 +551,27 @@ export default function Image( {
const showBlockControls = showUrlInput || allowCrop || showCoverControls;
+ const mediaReplaceFlow = (
+ <>
+ { console.log(
+ isSingleSelected && ! isEditingImage && ! lockUrlControls
+ ) }
+ { isSingleSelected && ! isEditingImage && ! lockUrlControls && (
+ <BlockControls group="other">
+ <MediaReplaceFlow
+ mediaId={ id }
+ mediaURL={ url }
+ name={ ! url ? __( 'Add image' ) : __( 'Replace' ) }
+ allowedTypes={ ALLOWED_MEDIA_TYPES }
+ accept="image/*"
+ onSelect={ onSelectImage }
+ onSelectURL={ onSelectURL }
+ onError={ onUploadError }
+ />
+ </BlockControls>
+ ) }
+ </>
+ );
const controls = (
<>
{ showBlockControls && (
@@ -587,19 +608,6 @@ export default function Image( {
) }
</BlockControls>
) }
- { isSingleSelected && ! isEditingImage && ! lockUrlControls && (
- <BlockControls group="other">
- <MediaReplaceFlow
- mediaId={ id }
- mediaURL={ url }
- allowedTypes={ ALLOWED_MEDIA_TYPES }
- accept="image/*"
- onSelect={ onSelectImage }
- onSelectURL={ onSelectURL }
- onError={ onUploadError }
- />
- </BlockControls>
- ) }
{ isSingleSelected && externalBlob && (
<BlockControls>
<ToolbarGroup>
@@ -1001,12 +1009,18 @@ export default function Image( {
}
if ( ! url && ! temporaryURL ) {
- // Add all controls if the image attributes are connected.
- return metadata?.bindings ? controls : sizeControls;
+ return (
+ <>
+ { mediaReplaceFlow }
+ { /* Add all controls if the image attributes are connected. */ }
+ { metadata?.bindings ? controls : sizeControls }
+ </>
+ );
}
return (
<>
+ { mediaReplaceFlow }
{ controls }
{ img }
Thanks @t-hamano I have worked on it as per your suggestion. I apologize for the delay, it was a busy week. Could you please review the changes at your convenience?
Also @jasmussen I tried to remove that initial flash by reversing the logic but couldn't completely remove that can you please look into if this will work for now.
Thanks 🙇
Thanks for the update! This PR is working well, but I think we need to find a way to avoid overriding many component styles.
From my understanding, the expected behavior of the placeholder in this PR is as follows:
- Display the illustration when the Image block is not selected
- When the placeholder is small, i.e. when it has the
is-smallclass, the illustration will always be displayed regardless of whether the Image block is selected or not.
Currently, the withIllustration prop is always true, so there are quite a few CSS overrides to achieve the above conditions.
Ideally, I think we need to dynamically control the withIllustration prop. If we could do that, we should be able to remove all the CSS that exists here.
That is, I want to achieve something like this:
export function ImageEdit( { isSelected: isSingleSelected } ) {
// ...
const placeholder = ( content ) => {
// There is no function called getBlockWidth...
const containerWidth = getBlockWidth();
const isSmallContainer = containerWidth < 160;
return (
<Placeholder
withIllustration={ ! isSingleSelected || isSmallContainer }
label={ __( 'Image' ) }
>
{ content }
</Placeholder>
);
};
// ...
}
As far as I know, the only way to know the width of a placeholder from outside the placeholder is to use the useResizeObserver hook.
@WordPress/gutenberg-components If you know of any other way to achieve the above approach other than using useResizeObserver, please let us know 🙏
As far as I know, the only way to know the width of a placeholder from outside the placeholder is to use the
useResizeObserverhook.
Sounds good to me for the purpose of this PR, and the amount of custom CSS we can shave off seems worth it. We should maybe consider abstracting it into Placeholder itself somehow, but for now a resize observer here feels warranted.
@mirka Thanks for the feedback!
@vipul0425 Sorry for changing the approach so often, but could you try the approach using useResizeObserver?
- Remove most (or all) of this CSS
- Use the
useResizeObserverhook to get the width of thePlaceholdercomponent - If the width is below a certain value, consider it a small container size
- Dynamically enable the
withIllustrationprop of the Placeholder component (withIllustration={ ! isSingleSelected || isSmallContainer }) - Some kind of workaround may be necessary to prevent the glitch pointed out in this comment.
Hi @t-hamano, I've updated the logic as you suggested. I used the reverse logic for the small container to prevent the initial flash of content. However, I noticed that this is causing two e2e tests to fail. If I'm understanding this correctly, this is due to the focus shifting to the image component instead of the Upload button on initial render.
If I remove the isLargeContainer && content condition inside the placeholder, the focus shifts back to the Upload button, but then the buttons also appear on the small container, which we don't want.
Do you have any suggestions? I tried manually focusing on the upload button when the component loads, but that approach didn't seem ideal.
@t-hamano apologies for the direct ping, do you still have bandwidth to look at the e2e tests? Otherwise I might be able to find someone else that can help, let me know!
@jasmussen Sorry for the late reply.
From what I can see, the E2E test failure is definitely as described in this comment.
That is, after an Image block is inserted, the Upload button should be focused, as shown below:
However, in this PR, to prevent flashing the white placeholder in narrow columns like the one below, the placeholder is initially rendered with the illustration and no focusable elements:
https://github.com/user-attachments/assets/3cb45d00-95de-4f89-a397-6ee6da7ae1be
So after the Image block is inserted, the Image block itself gets focus, not the Upload button:
We could modify the E2E tests themselves to fit this PR, but that might not be ideal. Because it would mean making an exception to the specification that is common to all blocks: "focus the first focusable element when a block is inserted."
As of now, I have not found an approach to solve this problem, so if anyone knows a good approach on what changes to make to this PR to achieve the following, I would appreciate your help 🙇♂️
- In wide columns, the upload button gets focus when an image block is inserted.
- In narrow columns, the image block placeholder always renders with an illustration and does not cause a flash.
Thanks so much for the clarification.
I wonder: can we extract the little trick that changes the focusing to avoid the brief flash? It's one we can live without, at least initially. Fixing that flash may not be worth extra code complexity, and there might be a different approach to placeholders worth looking at separately.
One suggestion would be to revert the illustration changes from this PR, i.e. just add an upload button to the toolbar. Adding an upload button would at least solve the issue of not being able to upload images in narrow columns.
Also, the problem we are trying to solve now will likely be encountered when applying a similar approach to other blocks (Media & Text block, Video block, etc.) in the future.
So maybe we can address this issue as a follow-up to find a more ideal solution.
What do you think?
I would hate to lose the fact that the placeholder functions in narrow contexts, it's entirely broken at the moment. What I'm suggesting is we rewind back to maybe this state of the PR (if that helps), where everything seemed to work and the only problem was the brief flash of the full placeholder when inserting from the slash command in a narrow context. The reason being: that insertion is an edge-case, not too common. But the image functioning in narrow contexts is arguably common and would be fantastic to fix.
What I'm suggesting is we rewind back to maybe this state of the PR (if that helps), where everything seemed to work and the only problem was the brief flash of the full placeholder when inserting from the slash command in a narrow context.
If we can tolerate this edge case, we can move forward with this PR 👍 As for the flash issue, we can look into an ideal solution in the future.
@vipul0425 Could you update this PR to move it forward, i.e. pass the E2E tests?
This means the following changes should be made:
- Preferentially render a white placeholder instead of an illustrated placeholder. This means changing
isLargeContainertoisSmallContainer. - Update any conditional statements that use
isLargeContainerto useisSmallContainerand invert the condition. This will cause flashing in the narrow columns, but should pass the E2E tests.
Apologies for the frequent changes 🙏
Thank you so much @t-hamano for your suggestions and for helping me move this forward 🙏. I’ve made the necessary changes, I’ll also try to address fixing the flashing issue alongside the E2E tests. If successful, I’ll create a follow-up PR for the fix.