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

Gutenlypso: Preserve block attributes when updating media #29791

Merged
merged 3 commits into from
Jan 1, 2019

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Dec 28, 2018

Our custom Media Modal for Gutenlypso relies on MediaStore.on( 'change' ) for updating the media items when they are edited within the Media Modal itself.

But the current approach is triggering an undesired effect: it resets the block attributes to default values (using the mediaCalypsoToGutenberg utility) causing the previous values to don't be preserved.

Changes proposed in this Pull Request

  • Move the current change listener out of the Media Modal, so the callback is executed only once per post, instead of once per block.
  • Create new class containing the whole logic that updates the media.
  • If the media changes, update the needed block attributes without modifying the rest of them.

Know issues

This PR doesn't update the media used as featured media for the post or for a simple payment block. A follow-up PR will cover this.

Testing instructions

  1. Open the block editor at /block-editor and select a site under a paid plan (free plans don't allow to edit images in the Media Modal).
  2. Insert an image block and click on the "Media Library" button.
  3. Select any image from the Media Modal (or upload a new one and select it).
  4. Adjust the image size and enter a caption.
  5. Repeat steps 2-4 with other media blocks and selecting the same image as selected in step 3: Gallery, Cover, Media & Text, Tiled Gallery.
  6. Select any of the current blocks and click on the Pencil icon on its toolbar.
  7. Click on the Edit button in the Media Modal and apply some changes to the image (e.g. flip it).
  8. Close the Media Modal without inserting the modified image.
  9. Make sure that all the media blocks show the modified image.
  10. Check the size and the caption of the previous image block are preserved in both the editor and the post preview.

Editor

Before After
image image

Preview

Before After
image image

Fixes #29557

@mmtr mmtr added [Type] Bug [Pri] High [Status] In Progress [Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Goal] Gutenberg Working towards full integration with Gutenberg labels Dec 28, 2018
@mmtr mmtr self-assigned this Dec 28, 2018
@mmtr mmtr requested a review from a team December 28, 2018 17:09
@matticbot
Copy link
Contributor

@mmtr mmtr changed the title Gutenlypso: Update edited images within the Media Modal Gutenlypso: Preserve block attributes when updating media Dec 31, 2018
@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 31, 2018
@Aurorum
Copy link
Contributor

Aurorum commented Dec 31, 2018

Just wanted to confirm that this fixes the original issue for me! It also tested well with the Image, Gallery, Inline Image, Cover and Media & Text blocks. 🎉

This PR doesn't update the media used as featured media for the post or for a simple payment block. A follow-up PR will cover this.

I can't test the Simple Payment block, but for some reason, this PR does fix the issue with Featured Images causing captions and alignments to not be preserved for me.

My bad, misunderstood!

};

// Block name -> Function that updates the media
mediaBocks = {
Copy link
Contributor

Choose a reason for hiding this comment

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

An explicit mapping works in the short term 👍

We may want to chat with @Automattic/luna and core folks, maybe @gziolo @aduth, if there's a way to detect blocks that use images, and maybe provide a decent fallback option

Copy link
Member

Choose a reason for hiding this comment

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

It looks like in core we don't refresh content at all when any change in the media library happens. I tried the following:

  • delete an image using media library, images are missing as soon as you refresh the page
  • edit an image using media library - only the image in the currently selected block gets updated, however the old image still works properly for other blocks

So, the short answer is we don't have support for it at the moment at all :(

Regarding finding the ways, I don't think it is straightforward because urls are stored as strings in attributes section and we simply inline components which handle them. I know that there were some work to strip out image ids when exporting/importing reusable blocks, so maybe @mcsf or @youknowriad would know more how to automate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @gziolo for the extra context. I suppose we'll keep thinking about this for core's use case as well. 🤔Maybe we could build something that could query for block functionality. Say, of an editors registered blocks, which ones involve the media library, and it'd return this list or something similar. Or may return a list of blocks instances that satisfy the feature query. I'm guessing we may have other use cases like that.

import MediaStore from 'lib/media/store';

const { updateBlockAttributes } = dispatch( 'core/editor' );
const { getBlocks } = select( 'core/editor' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we'll move away from a single editor store, but I think this usage is okay for now.

@gwwar
Copy link
Contributor

gwwar commented Jan 1, 2019

One small corner case we can follow up in, in another PR is that the featured image preview doesn't update if we edit an image in the media modal.

Sorry missed the known issues line 👍

screen shot 2018-12-31 at 4 47 10 pm

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation and analysis @mmtr ! This one was particularly tricky to get a solution in that covers most cases.

Thank you @torres126 for the report and help testing! Much appreciated. ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Goal] Gutenberg Working towards full integration with Gutenberg [Pri] High [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants