-
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
List View: Add media previews to list view for gallery and image blocks #53381
Conversation
Size Change: +373 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
*/ | ||
import { store as blockEditorStore } from '../../store'; | ||
|
||
function getImageUrl( block ) { |
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.
Not for this PR, but it's be good to think through an API that goes beyond hard-coding core blocks and builds similarly to the custom label support. For example, it might be a side effect of attribute types, so that a Cover block can show an image if one is set as well.
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.
Oh, thanks for taking an early look! Yes, I've just hard-coded things in at the moment while I hack around, but it'd be preferable in the longer-term to have a consistent API for it.
I imagine we'd likely keep things fairly hard-coded for the first round, but thinking through an API could be handy so that custom blocks / plugins can have image previews, too.
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.
Indeed, one step at a time :)
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.
Maybe in addition to the save
and edit
functions there could be a list
function or something that returns JSX that appears in the List View. This could replace __experimentalLabel
.
list( { attributes } ) {
return (
<Row>
__( 'Image' )
<ImagePreview attributes={ attributes } />
</Row>
);
}
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.
Hrm, interesting idea! One thing we'd probably want to be careful about is that it's quite a restricted area visually, so we might not want custom blocks to arbitrarily add elements, but rather provide data that the UI already knows how to display without anything overflowing or breaking out of the available space? There isn't very much real estate available, and the text truncation, etc for __experimentalLabel
is handled automatically by the list view rather than individual blocks having to worry about it.
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 a similar problem to BlockToolbar
. In theory you can put anything inside a <BlockControls>
and it will appear in the toolbar but we trust that extenders use the components we provide that are designed for this purpose e.g. ToolbarButton
.
In fact the problems are so similar that maybe instead of a list()
function we just have ListViewControls
which is rendered by edit()
😀
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 fact the problems are so similar that maybe instead of a list() function we just have ListViewControls which is rendered by edit() 😀
Yeah, something like that could work. Though I'd imagine that would likely be in addition to a label function? __experimentalLabel
works for other contexts than just the list view, since it winds up being called internally by useBlockDisplayTitle
.
In any case, definitely worth hacking around with in follow-ups to see if it's more ergonomic for the block code to decide what's rendered, or for the block code to provide data that the list view renders in an opinionated way.
Alrighty, I think this is ready for design feedback / review now. @jameskoster is this roughly what you had in mind for this one? Happy to adjust any of the styling, I mostly put it together based on the screenshot in #46015, but I might have gotten some of the numbers wrong for overlap, border-radius, etc 🙂 |
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.
This worked well for me, it's a great improvement. I agree that having a +N after the thumbs of gallery images if there are more would be useful.
Screen.Recording.2023-08-08.at.3.21.09.PM.mp4
Nice one, thanks for testing, folks! I'll tweak the styling a bit — I really like the idea of the +N idea, too, I'll give that a try 🙂 |
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.
This is working well for me so far!
.block-editor-list-view-block-select-button__images { | ||
display: flex; | ||
align-items: center; | ||
flex-direction: row; |
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.
row
is the default direction so we shouldn't need to specify it here, unless I'm missing something? align-items
also shouldn't be needed if the images are always the same size (and if they aren't, would it be better to leave it as stretch
so they all take up the same height?)
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.
Oh, thank you! Those were stray rules left over from hacking around, I'll remove them 🙂
} | ||
} | ||
|
||
function getImagesFromGallery( block ) { |
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.
Would it make sense to expand this to other container blocks that may have images, such as Group or Column? (Not necessarily as part of this PR)
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.
Yes — I was thinking we'd start with Gallery + Image for now, and look at other blocks such as Cover, etc in a follow-up where we can look at how an API for it might work, as in the other discussion above.
I like the idea of Group, Column, or other container blocks also showing what's in them, that's a cool idea! And if the API discussion winds up being a bit complex, then it'll be a simple follow-up to add hard-coded rules to allow Group, Column, etc in, too.
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.
We can try it, but it may be more distracting than necessary to have images also visible in parent group/columns blocks, and not just image/media related blocks.
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'd keep it just for galleries and blocks that can have an image as an attribute (Cover, Media and Text).
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.
Wrote a follow-up issue to add support for other image-attribute-based blocks: #53684
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.
Nice, that sounds good to me 👍
2b8a01c
to
035bdf8
Compare
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.
Great stuff. This is working well for me.
✅ Displays for image and gallery blocks only
✅ Handles multiple and nested galleries on a page pretty well (I tried 10)
✅ Looks the same in FF, Safari, Chrome and Edge
+1 on the +N indicator
Alrighty, thanks for all the testing, folks! 🙇 I think this is ready for another design review now. @jameskoster I've done the following:
Here's how it's looking (I included an example with the Lock icon so we can see how the text feels next to other icons): Happy to keep tweaking, and let me know if there are any specific values you'd like to go with for any of the above, of course 🙂 |
<span className="block-editor-list-view-block-select-button__image-count"> | ||
{ sprintf( | ||
/* translators: %d: Number of additional images within a block. */ | ||
__( '+%d' ), |
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.
This is really just a plus with a number, does it need translation? And if we assume the plus could be translated to something with more than one character, would it fit in the available space?
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 mostly about whether or not it's to the left or to the right of the number, rather than being translated to another character. Happy to remove the translation call if we think it's not needed, though!
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'd tend towards keeping the translation just in case some locales use a different symbol or order.
No strong opinion, but I think we can do without the number count on collapsed galleries, we could also probably show less images as well—it's just a cue. |
Yeah I think we can simplify. A couple of images are probably adequate in helping folks identify a given gallery, which is the main purpose of this affordance. Displaying only three thumbnails and removing the count will reduce the overall noise a lot. Sorry for the detour with the count! |
Sounds good, and no apology necessary! I think we only find the sweet spot with some of these things once we throw everything in there and see what feels good. I'll update 🙂 |
Seeing the numbers now I do agree it's a bit much. :D |
Update: I've removed the image count, and reduced it to a max of 3 images. Here's how it's looking now: I think this is ready for a final review now, @jameskoster! Let me know if there's anything else you'd like to tweak. |
17b45ae
to
ab9a910
Compare
ab9a910
to
39819e2
Compare
This is looking good :) |
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.
Nice! It's working well for me in post and site editors.
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.
Retested and working well, looks clean!
Thanks for the collaboration and reviews, everyone! 🙇 |
What?
An exploration into #46015 — add media previews for gallery and image blocks in the list view.
Why?
In large documents with many images or galleries, adding media thumbnails to the list view row should (hopefully) make it easier to identify which image or gallery is which, and improve navigating around documents within the list view.
In this PR:
How?
cover
useListViewImages
hook), but in follow-ups we can refactor the internals to use a block API of some kind (i.e. either via something inblock.json
, or via a method like__experimentalLabel
but for images)To-do
Testing Instructions
Screenshots or screencast
The following screenshot includes: