Skip to content
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

Update selected featured image ID on select #4453

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Mar 27, 2020

Summary

Set and update the featured image ID so that it can be marked as selected in the Media Library.

Demo

Before

After

Fixes #4414.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Mar 27, 2020
@westonruter
Copy link
Member

Build for testing: amp.zip (v1.5.0-RC1-20200327T063343Z-1a4e33c45)

id = wp.media.view.settings.post.featuredImageId;
let attachment;

if ( '' !== id && -1 !== id ) {
Copy link
Collaborator

@adamsilverstein adamsilverstein Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the post type doesn't support featured images, featuredImageId will be undefined here:

image

even if this code shouldn't load in that case, this should probably include a falsey check as a sanity check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Made suggested changes in 7cfd2d6.

Copy link
Collaborator

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. Left one comment about a possibly undefined variable. I haven't tested this code directly yet, assuming it fixes the reported issue, the approach looks good.

Copy link
Collaborator

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(one change requested)

@@ -57,47 +56,82 @@ export default ( InitialMediaUpload, minImageDimensions ) => {
*/
initFeaturedImage() {
const FeaturedImageSelectMediaFrame = getSelectMediaFrame( FeaturedImageToolbarSelect );

const FeaturedImageLibrary = wp.media.controller.FeaturedImage.extend( {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've extended wp.media.controller.FeaturedImage instead since it already has all the required functionality for updating the featured image ID when being selected, and pushing the selected image to the front of the image collection when being shown in the modal .

} ),
],
state: 'featured-image',
states: [ new FeaturedImageLibrary(), new wp.media.controller.EditImage() ],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the EditImage controller here to the state since it was a missing feature.

});
}, this.frame );

this.frame.on( 'open', this.onOpen );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the featured image has been changed but not saved, it was not being pushed to the front of the image collection. Hooking into the open event here allows for that to happen.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pierlon Can you explain why we need the image pushed to the front of the collection when this happens or what behavior this fixes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is trying to match the core behavior - I see now that it moves the selected item to the top of the list which is nice. It does something funny though if you close the modal with another image selected, then reopen it - the wrong image is moved to the front. Screencast: https://share.getcloudapp.com/12u1n5Yo

Testing with the plugin disabled I see the exact same behavior, so I guess this is fine for now (although it would be great to fix this bug upstream - probably if the user cancels the selection, the original item should be reselected)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply; yes it was to match the behavior of Core.

It does something funny though if you close the modal with another image selected, then reopen it - the wrong image is moved to the front.

Interesting... yes it would be best to have the bug fixed upstream. I'll file an issue in the Gutenberg repo.

Copy link
Collaborator

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @pierlon!

@westonruter westonruter added this to the v1.5.2 milestone Apr 1, 2020
@westonruter
Copy link
Member

Fix confirmed by user: #4414 (comment)

@westonruter westonruter merged commit 632e600 into develop Apr 2, 2020
@westonruter westonruter deleted the fix/4414-select-image-from-media-library branch April 2, 2020 22:36
westonruter pushed a commit that referenced this pull request Apr 2, 2020
* Update selected featured image ID on select

* Add sanity check for featured image ID being possibly undefined

* Ensure the featured image is the first item in the collection
@amedina
Copy link
Member

amedina commented Apr 3, 2020

This is not working for me. If I deactivate the plugin, it works. Activating the plugin (in any mode) makes this feature of WP to go away)

@amedina
Copy link
Member

amedina commented Apr 3, 2020

QA: Verified that it works. It did not work without flushing the browser cache.

westonruter added a commit that referenced this pull request Apr 3, 2020
* tag '1.5.2':
  Bump 1.5.2
  Bump version to 1.5.1-RC1
  Cache response status and headers when fetching external stylesheets (#4509)
  Fix securing multi-line mustache templates (#4521)
  Add CSS monitoring time series to Site Health debugging info (#4519)
  Update hostname used for WordPress TV embeds to fix external HTTP requests (#4524)
  Fix processing of element child sanitization loop when invalid elements are replaced with children (#4512)
  Account for more YouTube URL formats (#4508)
  Update selected featured image ID on select (#4453)
  Raise default threshold for disabling CSS caching (#4513)
  Cast i-amphtml-intrinsic-sizer dimensions to integers (#4506)
  Only move meta tags to the head when required and add processing for meta[http-equiv] (#4505)
  Fix failing tests (#4507)
  Bump 1.5.2-alpha
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin is Breaking Media Library Image "Pre-selection"
5 participants