Block API: Introduce "local" attributes and use it for the image block #63076
Conversation
youknowriad
added
[Feature] Block API
youknowriad
requested review from
draganescu,
Mamaduka,
mtias,
noisysocks,
gziolo,
glendaviesnz,
ellatrix and
annezazu
youknowriad
requested review from
artemiomorales,
michalczaplinski,
geriux,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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. |
|
Size Change: +4.28 kB (+0.24%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
When did we start doing this? The last thing I remember was that we wait to set attributes until the image is uploaded until we set the URL attribute, and the blob was just a local state. That's how the undo trap was fixed too. I wonder what changed where we put blob URLs in the attributes? |
|
Super dumb idea: What if we saved images when the post is saved, and replace URLs server side as images are uploaded and URLs resolved? This would also fix any sort of undo issues. |
The file type block transformations. Currently, it can't be avoided there. |
To be honest, that's very old, almost from the start (when we implemented drag and drop files) |
Not dumb at all but I think it's way too complex to implement. It would be good if we had a way to "create entities" with temporary ids or things like that and refer to them in other places of our data (things like categories, tags, images, even patterns, ...) but it's a huge and very unclear undertaking. |
I'm not sure, why would you not want to duplicate a product block with a productID ? I'm wondering if there's something we're missing on the UX side here and that duplication is actually fine here. Do you have more use-cases in mind. Overall though, you I think it should be fine to have a |
|
I'm actually even hesitant to prevent these local attributes from being duplicated. Sure, in our current case it will result in two image blocks with the same images but uploaded twice but isn't that better than an empty image and an uploaded image from a user's perspective. |
|
For the widgetId and as implemented today, I think it can be seen as a "local" attribute but ultimately, I also wonder if our widgets editor is the right UI for these and I would love if in the future (I know it's not priority) we merge it with template parts as widget areas are template parts are actually the exact same thing, so I can see the site editor's approach to editing template parts used for widget editor too (we could even use the |
There was a problem hiding this comment.
I think it's okay to go with this approach and I was in favor of this way back with the other PR.
Let's 🚢
From #29693:
|
|
OK, but if we don't care about the Either something like 37d6855 which stores the blob in a module variable. Or set the |
|
Here's yet another duplication-related issue, #55823. |
|
@noisysocks I'm not disregarding the duplication issue but I think it's less important and unclear to me still. It feels like a separate concern to me at this point than local attributes. For the solution you propose, it doesn't work, because we want to be able to create blocks with these temporary attributes from outside the block itself (block transforms...) and |
gziolo
changed the title
|
I updated the title to use the As a follow-up, it would finally rename the field |
I still feel the UI should reflect the incomplete state of uploading media. If the media shows a spinner and the image is being uploaded, users should be prevented from publishing a post or from updating a published post. If they do so, the front end of the post will reflect a "broken" state where media is only partially uploaded. Even if revisiting the editor will automatically complete the upload, we should provide a better user experience by not letting users inadvertently publish broken content. See #39223 |
|
@adamsilverstein yes, I didn't close that issue because I also think there's value in letting the user know that there are things in progress. I think both things are necessary though. We need the data to be correct, which this PR does, and we need the UI to be controllable by blocks somehow which we don't have a good API for yet. |
Just to put another option out there, rather than preventing, the save button could still be clickable, but the save request could wait for the image upload to be complete before initiating. This way it could be almost imperceptible for users, except for saving taking a bit longer while the two requests complete. |
WordPress#63076) Co-authored-by: youknowriad <[email protected]> Co-authored-by: mtias <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: noisysocks <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: gziolo <[email protected]>
WordPress#63076) Co-authored-by: youknowriad <[email protected]> Co-authored-by: mtias <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: noisysocks <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: gziolo <[email protected]>
fabiankaegy
added
the
Needs Dev Note
|
Hey @youknowriad 👋 Would you be able to help write a Dev Note for this feature? We are tracking all the dev notes for 6.7 them in this tracking issue: #65784 We are hoping to have all drafts compiled by October 13th so we have some time to review before RC1 |
|
We should include this one with the "stabilize role dev note" but yeah I can write a section for it. |
|
Good call :) Will group them in the tracking issue 👍 |
Related #39223
Related discussion #49466.
What?
For some blocks (mostly media blocks), we need a way to create the blocks from transforms using temporary files. After the temporary image is inserted, we upload the image to the media library and upon success update the block with the Right URL.
So far, we were doing this using "blob urls" (local urls) in the URL attribute for images. The problem with this approach is that if you're fast enough and save your post while the image is still being uploaded, you might end up with "temporary urls" in the saved post. This should never be the case.
One potential solution that has been considered is to block "saving" when an upload is in progress. I think that's not ideal and doesn't address the root cause where anyone can actually trigger the saving programmatically.
The root cause is that we're actually putting local state (temporary urls) in the block attributes that are meant to be saved because we need immediate feedback (block insertion) when drag and dropping blocks. So a solution I'm proposing in this PR is to introduce the concept of "temporary block attributes":
serializefor a block, they are actually ignored and do not persist in the HTML.Note: There's probably some undo/related logic that need updating but I'm thinking that most of the time, when we actually set this attribute, we'll also be setting another non temporary attribute, so in most cases, there's no impact on undo/redo.
Note 2: I know that this idea has been discussed previously on multiple occasions, feel free to update the PR descriptions and link into any relevant issue or discussion.
Testing Instructions
1- Throttle your network speed (that way uploading images will take a long time)
2- Drag and drop an image file into the post editor.
3- Save the post immediately.
4- Open the post in a new tab and notice that the inserted image is actually empty.
5- Go back to the old tab, and wait until the upload completes.
6- Notice that the image block has been updated properly with the right URL.