xwp / unsplash-wp

GNU General Public License v2.0
9 stars 3 forks source link

Unable to Set Featured Image in Gutenberg when AMP plugin active #132

Closed bmattb closed 4 years ago

bmattb commented 4 years ago

Bug Description

Was using Block editor on Win10 / Chrome

Selected Set Featured Image. Selected Unsplash image. Was returned to editor but the Featured Image section at right "hangs" on the "loading" animation:

image-2020-04-22-16-32-54-174

Expected Behaviour

Editor should present thumbnail of the selected image indicating that the image has now been attached to the post.

Steps to reproduce

  1. Go to https://dev-unsplash.pantheonsite.io/
  2. Click on a post to add a featured image
  3. Scroll down to the Featured Image widget
  4. Select Add Featured Image
  5. Select Media Library
  6. Select Unsplash tab
  7. Find a featured image and select "Add Featured Image"

Screenshots

Additional context


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

bmattb commented 4 years ago

@nailah-elridge request from @spacedmonkey was to get support from Pierre on this.

bmattb commented 4 years ago

@pierlon can you provide us with an estimate on this or at least some direction on approach?

pierlon commented 4 years ago

Sure @bmattb, I provided a quick analysis of the issue below with a possible solution to it:

At the current moment, the Unsplash plugin is able to initialize its own state for the Featured Image Backbone view by overriding the MediaUpload React component in Gutenberg. This is done by using the editor.MediaUpload filter hook provided by Gutenberg.

The AMP plugin is also overriding the MediaUpload React component via this filtering method. The issue here is when the AMP plugin also filters the MediaUpload (or any plugin, really), the Unsplash state that was created earlier by the Unsplash plugin is lost, and so the Featured Image view reverts back to normal behavior. This unintentional change breaks the Unsplash plugin.

Luckily, the filter function provided by Gutenberg used accepts a priority parameter, which like the PHP API, determines the order in how the registered filters should be executed. In this case, neither the AMP nor Unsplash plugins provide this parameter, so they have the same default priority of 10.

If the Unsplash plugin sets a lower priority of the hook, that should resolve the issue here, for example:

--- assets/src/media-selector/featured-image-selector.js    (revision b31dcb598e8d62419040d4e73eb6086b158ff048)
+++ assets/src/media-selector/featured-image-selector.js    (date 1590527551291)
@@ -16,5 +16,6 @@
 addFilter(
    'editor.MediaUpload',
    'unsplash/extend-featured-image',
-   () => UnsplashMediaUpload
+   () => UnsplashMediaUpload,
+   20
 );

Additional testing would be required to ensure that this would not lead to more bugs, but that should be all :smile:.

bmattb commented 4 years ago

Thanks @pierlon !!

spacedmonkey commented 4 years ago

Moved to QA.

csossi commented 4 years ago

Verified in QA

bmattb commented 4 years ago

confirmed working as expected