PoC: Show media previews in the List View via new API #57349
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Type] Enhancement
|
Size Change: -8 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
t-hamano
force-pushed
the
poc/listview-block-image-api
branch
from
4b9ce9e
to
526605b
Compare
t-hamano
changed the title
t-hamano
force-pushed
the
poc/listview-block-image-api
branch
from
526605b
to
f35b97c
Compare
t-hamano
requested review from
ellatrix,
artemiomorales,
michalczaplinski,
geriux,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Flaky tests detected in f35b97c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7302253510
|
t-hamano
force-pushed
the
poc/listview-block-image-api
branch
from
f35b97c
to
9376f15
Compare
andrewserong
reviewed
Jan 2, 2024
There was a problem hiding this comment.
Oh, this is a clever idea, thanks for picking this one up @t-hamano!
Overall, I don't mind the idea of adding state to keep track of the block images, but I was wondering what the benefits or trade-offs might be between using state versus trying to find a declarative approach in block.json that defines where the block image is stored, or something along those lines.
What I had in mind was if it'd be possible to have something in block.json that specifies where to look for the block image. So the Image block might reference the image's id or external url, and the Gallery block might specify "children" or "innerBlocks" so that the list view knows to iterate over the first x number of blocks within the block in order to grab the image urls. That approach might have its own drawbacks or blockers, too.
Mostly, what I'm wondering is how could we simplify the API for it so that if and when we want to allow 3rd party blocks to expose images in the list view, it'd be as simple as possible for them to do so.
I suppose from my perspective, one of the drawbacks of using a useEffect to call setBlockImage is that it means the block itself has to know about its own images and what to do with them, so things like the background image block support won't be exposed automatically. But one of the benefits is that blocks like the image block can grab the right URL from an ID, etc, and in that case the block itself could be the right place to do it 🤔
Could there even be an in-between approach where we have a default behaviour that happens within useBlockProps (inferred via block.json, perhaps), but folks can pass a blockImage prop to it to override when needed?
It's an interesting problem! What are your thoughts about the approach and how it might work for other blocks / factoring in things like the background image support? I might just ping @youknowriad for visibility, too, given recent discussions about the editor APIs.
|
Thank you for your feedback, @andrewserong!
I was also concerned about this. While this PR approach gives us the flexibility to expose list view image sources, it can be a bit unintuitive, especially for third-party block developers😅
Ideally, this would certainly be easier to understand and use. For example, if we want to expose the {
"name": "core/image",
"supports": {
"listViewImage": {
"attribute": "url"
}
}
}Or if we want to convert to image URL based on media Id: {
"name": "core/image",
"supports": {
"listViewImage": {
"attribute": "id",
"type": "mediaId"
}
}
}Or if we want to expose list view images based on child block attributes in the Group block: {
"name": "core/group",
"supports": {
"listViewImage": {
"type": "children",
"blocks": [ "core/image" ],
"attribute": "url"
}
}
}I'd like to hear everyone's opinions on what approach is best. |
|
Nice idea, and I agree with @andrewserong's comments here, I think if we are to add an API here, it should be something declarative in the register block function rather than a state. In a sense, the "image" is just another representation of a block
I'm sure I'm forgetting more, but it seems we may need Now, we already have I guess what I'm saying is that we should keep in mind all these precedents and think about something more generic if possible, if not, using the same approach and add a |
|
Thank you for your opinions, @andrewserong and @youknowriad. So, I'd like to close this PR and explore an approach to opt-in declaratively via |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: This PR is incomplete, so do not merge it.
Related to #53381
Hopefully #53684 will be resolved in the future.
What?
This PR allows the media preview in the List View implemented in #53381 to be used in blocks other than the Image block and the Gallery block.
If this approach makes sense, I would like to consider the following:
getImageUrl,getBlockMediaPreviewetc.) and specificationsPlease let me know if this approach makes sense.
Why?
Media preview in the List View is currently limited to the Image block and the Gallery block only. To apply this feature to other blocks such as Cover, Media & Text, including third-party blocks, we cannot rely on block names or specific attributes.
How?
To make this functionality universally available, I added new APIs:
getBlockImage/setBlockImage. If we want to add this media preview to other blocks in the future, we could use the following implementation.Testing Instructions
The Gallery and Image blocks that support media previews have been updated to use this new API. The behavior in the List View should remain as before.