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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions client/gutenberg/editor/hooks/components/media-upload/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@
*/
import React, { Component, Fragment } from 'react';
import { connect } from 'react-redux';
import { debounce, includes, isArray, map } from 'lodash';
import { includes, isArray, map } from 'lodash';

/**
* Internal dependencies
*/
import MediaLibrarySelectedData from 'components/data/media-library-selected-data';
import MediaModal from 'post-editor/media-modal';
import MediaActions from 'lib/media/actions';
import MediaStore from 'lib/media/store';
import { getSelectedSiteId } from 'state/ui/selectors';
import { mediaCalypsoToGutenberg } from './utils';

Expand All @@ -22,9 +21,6 @@ export class MediaUpload extends Component {
};

componentDidMount() {
if ( includes( this.props.allowedTypes, 'image' ) ) {
MediaStore.on( 'change', this.updateMedia );
}
MediaActions.setLibrarySelectedItems( this.props.siteId, this.getSelectedItems() );
}

Expand Down Expand Up @@ -58,19 +54,6 @@ export class MediaUpload extends Component {
}
};

updateMedia = debounce( () => {
const { multiple, siteId, value } = this.props;
if ( ! value ) {
return;
}
const media = {
items: multiple
? map( value, id => MediaStore.get( siteId, id ) )
: [ MediaStore.get( siteId, value ) ],
};
this.insertMedia( media );
} );

onCloseModal = media => {
if ( media ) {
this.insertMedia( media );
Expand Down
3 changes: 3 additions & 0 deletions client/gutenberg/editor/main.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import refreshRegistrations from '../extensions/presets/jetpack/utils/refresh-re
import { getSiteOption, getSiteSlug } from 'state/sites/selectors';
import { getPageTemplates } from 'state/page-templates/selectors';
import { MimeTypes } from 'lib/media/constants';
import autoUpdateMedia from './utils/media-updater';

/**
* Style dependencies
Expand All @@ -53,6 +54,8 @@ class GutenbergEditor extends Component {
refreshRegistrations();

setDateGMTOffset( gmtOffset );

autoUpdateMedia( siteId );
}

componentDidUpdate( prevProp ) {
Expand Down
148 changes: 148 additions & 0 deletions client/gutenberg/editor/utils/media-updater/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/** @format */

/**
* External dependencies
*/
import { dispatch, select } from '@wordpress/data';
import { debounce, partialRight } from 'lodash';

/**
* Internal dependencies
*/
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.


/**
* Update the previewed media on the post after changing it on the Media Modal
*/
class MediaUpdater {
siteId = null;

constructor( siteId ) {
this.siteId = siteId;
MediaStore.on( 'change', () => {
this.updateMediaBlocks( getBlocks() );
} );
}

/**
* Update the media of the blocks rendering an edited image
* @param {Array} blocks Array of block objects for the current post being edited
*/
updateMediaBlocks = debounce( blocks => {
blocks.forEach( block => {
if ( this.mediaBocks[ block.name ] ) {
this.mediaBocks[ block.name ]( block );
}

if ( block.innerBlocks.length ) {
this.updateMediaBlocks( block.innerBlocks );
}
} );
} );

/**
* Preload an image given its URL so it can be rendered without flickering
* @param {String} imageUrl URL of the image to preload
* @returns {Promise} Promises that resolves when the image is preloaded
*/
preloadImage = imageUrl =>
new Promise( ( resolve, reject ) => {
const preloadImage = new Image();
preloadImage.src = imageUrl;
preloadImage.onload = resolve;
preloadImage.onerror = reject;
} );

/**
* Update the block attribute storing a single image URL
* @param {Object} block Block to update
* @param {Object} attrNames Mapping defining the name of the attributes to update
*/
updateSingeImageBlock = async ( block, attrNames = {} ) => {
attrNames = {
id: 'id',
url: 'url',
...attrNames,
};

const { id, url } = attrNames;
const { siteId } = this;
const { clientId, attributes } = block;

const media = MediaStore.get( siteId, attributes[ id ] );

if ( media ) {
// If image was deleted in the Media Modal, we delete it in the block.
if ( media.status === 'deleted' || ! media.URL ) {
updateBlockAttributes( clientId, {
[ id ]: undefined,
[ url ]: undefined,
} );
} else {
// To avoid an undesirable flicker when the image hasn't yet been loaded,
// we preload the image before rendering.
await this.preloadImage( media.URL );

updateBlockAttributes( clientId, { [ url ]: media.URL } );
}
}
};

/**
* Update the block attribute storing a list of images
* @param {Object} block Block to update
* @param {Object} attrNames Mapping defining the name of the attributes to update
*/
updateMultipleImagesBlock = async ( block, attrNames = {} ) => {
attrNames = {
images: 'images',
id: 'id',
url: 'url',
...attrNames,
};

const { images, id, url } = attrNames;
const { siteId } = this;
const { clientId, attributes } = block;

const currentImages = attributes[ images ];
const updatedImages = [];

for ( const image of currentImages ) {
const media = MediaStore.get( siteId, image[ id ] );

// Deleted images are not included in the new images attribute so
// they are removed from the block.
if ( media && media.status !== 'deleted' && media.URL ) {
// To avoid an undesirable flicker when the image hasn't yet been loaded,
// we preload the image before rendering.
await this.preloadImage( media.URL );

updatedImages.push( {
...image,
[ url ]: media.URL,
} );
}
}

updateBlockAttributes( clientId, { [ images ]: updatedImages } );
};

// 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.

'core/cover': this.updateSingeImageBlock,
'core/image': this.updateSingeImageBlock,
'core/file': partialRight( this.updateSingeImageBlock, { url: 'href' } ),
'core/gallery': this.updateMultipleImagesBlock,
'core/media-text': partialRight( this.updateSingeImageBlock, {
id: 'mediaId',
url: 'mediaUrl',
} ),
'jetpack/tiled-gallery': this.updateMultipleImagesBlock,
};
}

export default siteId => new MediaUpdater( siteId );