-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Gallery block: Move add/edit media to block toolbar #38036
Gallery block: Move add/edit media to block toolbar #38036
Conversation
Size Change: +669 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
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 tests well for me and seems like a great improvement 😄
- Adding a single image works
- Adding multiple images works
- No regressions seen with the MediaUpload & Replace flows with individual Image blocks inside/outside of the gallery, or with the Video block
- The new toolbar button is not present in the placeholder state (when the Gallery is empty)
- You can still add images from the placeholder state
This feels much better in the toolbar than fixed to the bottom of the block, since it scrolls with you and is always visible when the block is selected, especially in large galleries. If you do choose to Add
images while scrolled to the top of the block, though, it may not be apparent that the operation was successful, because the images actually get inserted at the end of the gallery. That is what I'd expect, but maybe it would be helpful to automatically scroll to the bottom of the block when images are added this way?
Here's an example:
gallery-toolbar-inserter.mov
Good idea, I will take a look at doing that. |
…r, add a scroll to the new blocks added
@ramonjd, @stacimc I have added a scroll to the first of the latest images, but Ramon your suggestion of just selecting the first of these might avoid the need to pull on the new scroll library dependency - I will have a play with that approach. |
I didn't search around much, but it looks like we'd get it out of the box of block detects that it's selected: |
Selecting the block is an even better idea IMO ✨ It addresses the scrolling issue and, now you mention it, also feels like appropriate behavior after adding new images. If it ends up being tricky to get working I think it could always be an improvement/follow-up, but I think that would be great! |
@stacimc, @ramonjd with selecting the blocks we have two options:
any views on which of these is the best UX? Both of these approaches are committed if you want to try locally - single, multiple |
Thanks for trying out both options @glendaviesnz Both work as described in the screencasts above for me. I don't have a definitive answer on this one, but I asked myself: what do I want to do after I've added a single or multiple images? Definitely see what the gallery looks like. 😄 Just having the auto-scroll in place makes a huge difference already.
For first-item selected, it's curious to see the caption focussed. I can't reproduce that when selecting blocks from the list view. list-select.mp4Still, it's consistent between adding one block vs adding multiple, and it brings me to the point at which the images are selected and, with one click, I can carry on even if I rarely want to add a caption.
When I add a few blocks at a time the Block Inspector reveals an empty Image settings panel so there's not much I can really do any way except toggle the border radius. This is current behaviour for the image block anyway. One advantage is that I easily delete the newly-added image all at once. But what makes me prefer this option is when I add one image only: it selects the individual block and I can do a lot more straight away. For me, the scroll-to improvement outweighs any select behaviour, which I guess we can tweak/swap later on which ever way we decide. |
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 an excellent improvement.
I've tested with multiple galleries, adding single and multiple images.
Adding/uploading images works as expected, and the page scrolls to the newly-added and selected images.
The 'select all' option after adding multiple images, I think, works well and doesn't distract from editing.
Thank you!
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 works great for me @glendaviesnz! I like that we're keeping the placeholder when there are no images, and this helps the presentation of the block when there are images look more closely like the front end of the block while editing it. Like the others mentioned, it's nice having the persistent ability to add images in a really tall gallery block, too, feels like a good improvement to me!
I also really like the behaviour where multiple added images are selected after adding them — makes it really clear which are the new ones:
Kapture.2022-01-19.at.14.02.15.mp4
Just left an optional comment about potentially optimising checking for allowed types, but otherwise the logic all looks good to me!
@@ -134,8 +154,13 @@ const MediaReplaceFlow = ( { | |||
<> | |||
<NavigableMenu className="block-editor-media-replace-flow__media-upload-menu"> | |||
<MediaUpload | |||
value={ mediaId } | |||
onSelect={ ( media ) => selectMedia( media ) } | |||
gallery={ multiple && onlyAllowsImages() } |
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 looks like this gets re-calculated on every render. I'm not sure if it's really needed as an optimisation, but if the value changes only when allowedTypes
changes, could we replace the definition of onlyAllowsImages
with a useMemo
call and store the result in a const?
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 don't think there was a performance issue here, but I have moved the function call out of the prop just to be sure. Because onlyAllowsImages
is inexpensive and returns a boolean I didn't think useMemo was 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.
It's probably not needed — I think I just had useMemo
on my brain after looking @aaronrobertshaw's recent efforts on improving performance in the ToolsPanel
. Just a good one for us to keep in the back of our minds in case we want to optimise anything further down the track. Looks good to me!
👋 We spotted an issue related to these changes in the native version of the Gallery block. It will be addressed in this PR, but since the changes also involve changes in the web version, I'd appreciate it if you could take a quick look, thanks for your help 🙇 ! |
I also noticed that uploading multiple images from the Not sure if you're already aware of the issue, if not I'll be more than happy to open a ticket 🙇 . Screen.Capture.on.2022-01-26.at.13-04-36.mp4 |
Description
Related to: #21247
Moves the Gallery block image add/edit from the placeholder at the bottom of the Gallery to a button in the toolbar.
This is the first step in updating the adding of images to the Gallery. The next step will be to enable the inline block inserter, but there are currently some issues with this not working in the Gallery horizontal layout that needs to be resolved.
How to test
Add a Gallery block and make sure that images can be added from the block placeholder, and also from the 'Add' button that appears in block toolbar once a Gallery contains images.
Screenshots