Gallery: Update media frame state management by joemcgill · Pull Request #2488 · WordPress/gutenberg

joemcgill · 2017-08-21T13:03:25Z

Following #2294, this updates the Media component so that existing galleries instantiate a media frame in the gallery-edit state with the correct attachment models passed in on creation, rather than overriding the state after the frame has been created.

This fixes several small bugs, like having text from the gallery state still showing in the toolbar, the cancel menu item not working as expected in the menu, etc.

This also reworks the way attachments are fetched so that we can get the entire collection of attachments in a single HTTP request, rather than separate requests for each attachment.

codecov · 2017-08-21T13:03:28Z

Codecov Report

Merging #2488 into master will decrease coverage by 16.89%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master    #2488      +/-   ##
==========================================
- Coverage      44%   27.11%   -16.9%     
==========================================
  Files         398      159     -239     
  Lines        9015     4902    -4113     
  Branches     1596      817     -779     
==========================================
- Hits         3967     1329    -2638     
+ Misses       4322     3030    -1292     
+ Partials      726      543     -183

Impacted Files	Coverage Δ
blocks/media-upload-button/index.js	`7.14% <14.28%> (ø)`
blocks/url-input/button.js	`0% <0%> (-100%)`	⬇️
components/panel/row.js	`0% <0%> (-100%)`	⬇️
components/keyboard-shortcuts/index.js	`0% <0%> (-92.86%)`	⬇️
blocks/color-palette/index.js	`0% <0%> (-90%)`	⬇️
blocks/alignment-toolbar/index.js	`16.66% <0%> (-83.34%)`	⬇️
blocks/block-controls/index.js	`0% <0%> (-80%)`	⬇️
components/drop-zone/index.js	`0% <0%> (-72.73%)`	⬇️
blocks/block-alignment-toolbar/index.js	`33.33% <0%> (-55.56%)`	⬇️
blocks/library/more/index.js	`30% <0%> (-51.49%)`	⬇️
... and 464 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d96d79b...4586353. Read the comment docs.

youknowriad · 2017-08-21T13:12:06Z

blocks/media-upload-button/index.js

 		onSelect( multiple ? attachment : attachment[ 0 ] );
 	}

-	onOpen() {


I guess this means we should also set a selection object when we're not in a gallery (Editing an image block is broken with this change).

Ah right. We do need to handle this the same way for images as well.

The problem was fixed with the new commit I added :)

youknowriad · 2017-08-21T13:13:21Z

blocks/media-upload-button/index.js

 		if ( gallery ) {
+			const ids = this.props.value || [];
+			const attachments = getAttachmentsCollection( ids );
+			const currentState = ( this.props.value ) ? 'gallery-edit' : 'gallery';


Should we do the same for a single media? I mean do we have something like 'edit' and 'select' states for single media selector?

Hi, @youknowriad we have edit states for single media like audio and video, but this states open with a modal to change details like sizes, captions etc of the item that we can now change in the Gutenberg block. So for single media I don't think we should use this states.

youknowriad · 2017-08-21T13:15:44Z

blocks/media-upload-button/index.js

@@ -78,11 +92,19 @@ class MediaUploadButton extends Component {
 		}

 		if ( gallery ) {


Isn't the gallery prop a duplicate of multiple prop?

Not exactly, because you could use a media upload button to insert multiple images that are not in a gallery. I would like to see us handle props differently for galleries than passing a gallery prop (see: #1979) but was going to leave that as a separate issue.

In this case, should we expect the selection object to be dependent on the multiple prop instead of the gallery prop. I'm guessing the selection shape depends on whether the selection is multiple or not, and not whether it's a gallery view or not?

I see what you're getting at. Yeah, we could probably use a similar method in both cases.

youknowriad · 2018-01-30T09:08:29Z

Any plans to update this. I'm thinking addressing at least this is important.

This also reworks the way attachments are fetched so that we can get the entire collection of attachments in a single HTTP request, rather than separate requests for each attachment.

--
Anything we can help with?

joemcgill · 2018-01-30T16:36:07Z

Hi @youknowriad, thanks for checking in. I clearly haven't had the time to circle back to this issue, but I agree that it would be nice to update/finish this off. I should have time in a few weeks to dig back into this issue if you're ok with leaving it open.

youknowriad · 2018-01-30T16:59:09Z

@joemcgill Sure, not urgent at all, just wanted to check in. Let's leave it open.

…ding of media frame when editing a gallery; Editing a gallery opens media frame in gallery-edit state This sets the editing option based on the values property of our component. Previously, we were replacing the media frame state right before opening the modal, but after views like the menu were created. This caused a few bugs, like the cancel button moving back to the initial "create gallery" state and some labels being incorrect. This creates a selection from any existing IDs and passes it to the frame before it is created to avoid these issues. Additionally, this updates the method for getting a collection of attachments using `wp.media.query` so we can fetch all of the attachment models in one request, rather than relying on seperate requests for each attachment in the gallery. When opening the `wp.media` modal to modify an existing gallery, the frame should be opened in the `gallery-edit` state, rather than the `gallery` state used when selecting images for a new gallery.

…d; Corrected bug where if gallery was empty all media content was loaded.

jorgefilipecosta · 2018-04-23T14:48:54Z

Hi @joemcgill, @youknowriad,
I think the changes in this PR are a must as they increase the performance, and make the gallery modal use the correct frame when editing instead of the create one.
I rebased this PR keeping exactly the same changes proposed by @joemcgill.
And after I added a new commit which addresses some problems I found:
Before all images of the MediaUpload were loaded even if we did not open the media modal, now they are loaded only when the Media modal opens.
Before if we added an empty gallery block, as we did not filter by id all the media attachments were loaded.
Problems using MediaUplod for a single item e.g: image block were solved. The logic was kept the same as before, but an improvement to not fetch the attachment if we already have it was added.

youknowriad · 2018-04-23T15:35:25Z

edit-post/hooks/blocks/media-upload/index.js

-			frameConfig.library = { type };
-		}

 		if ( gallery ) {


Still having trouble to understand the difference between gallery and multiple props. I wonder if we should consolidate these two or if they are useful separately.

Hi @youknowriad, the gallery flag indicates that we want to use the gallery media frame, the multiple still uses the normal select frame but allows us to select multiple files. Imagine a document block that wants to allow to select multiple documents it may use multiple flag but not use gallery flag.

youknowriad

LGTM 👍

joemcgill · 2018-04-24T16:32:18Z

Nice changes. Thanks for picking this up, @jorgefilipecosta!

* Gallery: Properly set editing option for the media frame; Improve loading of media frame when editing a gallery; Editing a gallery opens media frame in gallery-edit state This sets the editing option based on the values property of our component. Previously, we were replacing the media frame state right before opening the modal, but after views like the menu were created. This caused a few bugs, like the cancel button moving back to the initial "create gallery" state and some labels being incorrect. This creates a selection from any existing IDs and passes it to the frame before it is created to avoid these issues. Additionally, this updates the method for getting a collection of attachments using `wp.media.query` so we can fetch all of the attachment models in one request, rather than relying on seperate requests for each attachment in the gallery. When opening the `wp.media` modal to modify an existing gallery, the frame should be opened in the `gallery-edit` state, rather than the `gallery` state used when selecting images for a new gallery. * Corrected bug where single media selection was not working as expected; Corrected bug where if gallery was empty all media content was loaded.

mtias · 2018-05-03T11:56:08Z

Thanks for finishing this one!

joemcgill requested a review from mkaz August 21, 2017 13:03

youknowriad reviewed Aug 21, 2017

View reviewed changes

joemcgill mentioned this pull request Aug 25, 2017

Gallery Widget xwp/wp-core-media-widgets#120

Merged

bradyvercher mentioned this pull request Dec 15, 2017

Gallery blocks are slow #4032

Closed

youknowriad added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 30, 2018

jorgefilipecosta mentioned this pull request Apr 23, 2018

Improve performance of the gallery block, by removing unnecessary requests. #6325

Closed

joemcgill and others added 2 commits April 23, 2018 11:16

Corrected bug where single media selection was not working as expecte…

4586353

…d; Corrected bug where if gallery was empty all media content was loaded.

jorgefilipecosta force-pushed the update/gallery-block-media-frame-updates branch from 271fa19 to 4586353 Compare April 23, 2018 14:30

jorgefilipecosta removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Apr 23, 2018

youknowriad reviewed Apr 23, 2018

View reviewed changes

youknowriad approved these changes Apr 23, 2018

View reviewed changes

jorgefilipecosta merged commit d13a862 into master Apr 24, 2018

jorgefilipecosta deleted the update/gallery-block-media-frame-updates branch April 24, 2018 07:22

jorgefilipecosta mentioned this pull request Apr 24, 2018

Gallery block: editing a gallery uses "Insert gallery" button #4491

Closed

gziolo added this to the 2.8 milestone Apr 24, 2018

danielbachhuber mentioned this pull request Nov 9, 2018

Fetch all potential categories, attachments, and tags #10873

Closed

andrewserong mentioned this pull request Sep 23, 2021

MediaUpload: Ensure current images in a gallery are selected after opening media library #35070

Merged

7 tasks

		@@ -78,11 +92,19 @@ class MediaUploadButton extends Component {
		}

		if ( gallery ) {

Conversation

joemcgill commented Aug 21, 2017

Uh oh!

codecov bot commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Jan 30, 2018

Uh oh!

joemcgill commented Jan 30, 2018

Uh oh!

youknowriad commented Jan 30, 2018

Uh oh!

jorgefilipecosta commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

joemcgill commented Apr 24, 2018

Uh oh!

mtias commented May 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Aug 21, 2017 •

edited

Loading

youknowriad Aug 21, 2017 •

edited

Loading

jorgefilipecosta commented Apr 23, 2018 •

edited

Loading