Image: Adds the block controls for uploading image. #64320
Conversation
|
Nice! This makes good progress, but doesn't yet fully fix 54867. Quick GIF:
We need only two changes:
2 should be possible with CSS. Essentially it should look like this:
That maintains the gray placeholder material rather than the white
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 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. |
|
Nice. Took the latest for a spin:
In the above I added a
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 |
| const [ temporaryURL, setTemporaryURL ] = useState( attributes.blob ); | ||
|
|
||
| const [ resizeListener, sizes ] = useResizeObserver(); |
There was a problem hiding this comment.
Pretty sure this one is already included in the Placeholder component, so it may be best to make these width changes to that component itself, rather than locally here.
| const [ temporaryURL, setTemporaryURL ] = useState( attributes.blob ); | ||
|
|
||
| const [ resizeListener, sizes ] = useResizeObserver(); |
There was a problem hiding this comment.
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?
Technically, yes. But in the long run, I'm thinking it could be cleaner and more flexible if this kind of responsive styling was handled solely by the consumer of Placeholder, using CSS container queries (rather than a resize observer). That way, consumers wouldn't have to depend on a set of arbitrary breakpoints that Placeholder defines.
I'm not intimately familiar with the requirements of this Image block in particular, but on first glance I would say it could make it simpler and more performant. Could be worth trying here. (This is a non-blocking comment, just suggesting in case it hadn't been considered.)
There was a problem hiding this comment.
Agreed with Lena — this seems a good fit for CSS Container Queries
(edit: when I wrote my message, I hadn't seen @vipul0425 's reply, which was posted almost at the same time)
There was a problem hiding this comment.
@vipul0425 I don't have much insight into the current state of things, but it looks like you were able to achieve what you needed in this PR without resize observers or container queries, so yes it seems like something that can be addressed as a separate issue 👍
|
Hi @jasmussen, I have removed the @t-hamano, I have implemented the 
|
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! |
| .block-editor-media-placeholder:not(.is-small) { | ||
| .components-placeholder__fieldset, | ||
| .components-placeholder__label { | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
.components-* class names are meant to be a private implementation detail and should not be used outside of the components package.
Overrides like the one highlighter make this usage Placeholder component prone to future breakage, should the component update internally (either its classnames, or how its internal layout is organized).
Having said that, I can see that there are more examples of using private classnames and in general applying style overrides in this file, which is a strong indicator that we should completely rethink how the media placeholder is implemented. So, in that sense, my feedback shouldn't considered blocking for this specific PR, but rather a warning about how badly styles are architected for this component and the risks.
It feels like it's trying to bend the Placeholder component beyond its limits. Therefore:
- we should determine if there are any features that we can add to the
Placeholdercomponent so that consumers don't have to resort to style overrides and other hacky practices; - otherwise, we should re-implement the component in a way that builds on top of
Placeholder. For example, instead of settingdisplay:noneto.components-placeholder__illustration, why don't we pass thewithIllustration={false}prop? - finally, if the designs / feature are diverging too much, we should ask ourselves whether this warrants a new, separate component (in the block library package)
|
@vipul0425 Thanks for the update!
Sorry if my suggestion was unclear. What I meant is, I don't think we need to add another |
|
@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? |
|
How about making the following changes to this PR?
Diffdiff --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 } |
akasunil
added
[Type] Enhancement
github-actions
bot
removed
the
props-bot
|
@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: 357732875-aee02f3f-7d36-430d-ae3b-bd26621a72d0.online-video-cutter.com.mp4So 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 🙇♂️
|
|
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. |
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:
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. |
There was a problem hiding this comment.
Thanks for the update! I think everything is working as expected.
One last thing I noticed is that the Enter button for the "Current media URL" field is misaligned:
However, I believe this is an issue with the MediaReplaceFlow or LinkControl component itself, and may be addressed in #65158.
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
|
It appears to have passed all E2E tests 👍 |
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
Screenshots or screencast
For Large Size
For Medium Size
For small Size
