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.

Sign up for GitHub

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

Blocks: Fix image block error with .tiff images, refactor using withAPIData #2840

Merged
merged 1 commit into from
Oct 3, 2017
Merged

Blocks: Fix image block error with .tiff images, refactor using withAPIData #2840

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
82 changes: 40 additions & 42 deletions blocks/library/image/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,28 @@
*/
import classnames from 'classnames';
import ResizableBox from 'react-resizable-box';
import { startCase, findKey } from 'lodash';
import {
startCase,
isEmpty,
map,
get,
flowRight,
} from 'lodash';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { mediaUpload } from '@wordpress/utils';
import { Placeholder, Dashicon, Toolbar, DropZone, FormFileUpload } from '@wordpress/components';
import {
Placeholder,
Dashicon,
Toolbar,
DropZone,
FormFileUpload,
withAPIData,
} from '@wordpress/components';

/**
* Internal dependencies
Expand All @@ -35,27 +48,11 @@ class ImageBlock extends Component {
this.updateAlignment = this.updateAlignment.bind( this );
this.onSelectImage = this.onSelectImage.bind( this );
this.onSetHref = this.onSetHref.bind( this );
this.updateImageSize = this.updateImageSize.bind( this );
this.state = {
availableSizes: {},
};
}

componentDidMount() {
if ( this.props.attributes.id ) {
this.fetchMedia( this.props.attributes.id );
}
}

componentWillUnmout() {
if ( this.fetchImageRequest ) {
this.fetchImageRequest.abort();
}
this.updateImageURL = this.updateImageURL.bind( this );
}

onSelectImage( media ) {
this.props.setAttributes( { url: media.url, alt: media.alt, caption: media.caption, id: media.id } );
this.fetchMedia( media.id );
}

onSetHref( value ) {
Expand All @@ -73,31 +70,20 @@ class ImageBlock extends Component {
this.props.setAttributes( { ...extraUpdatedAttributes, align: nextAlign } );
}

fetchMedia( id ) {
Copy link
Member

Choose a reason for hiding this comment

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

HOCs remove so much code duplication 👍

if ( this.fetchImageRequest ) {
this.fetchImageRequest.abort();
}
this.fetchImageRequest = new wp.api.models.Media( { id } ).fetch();
this.fetchImageRequest.then( ( image ) => {
this.setState( {
availableSizes: image.media_details.sizes,
} );
} );
updateImageURL( url ) {
this.props.setAttributes( { url } );
}

updateImageSize( selectedSize ) {
this.props.setAttributes( {
url: this.state.availableSizes[ selectedSize ].source_url,
} );
getAvailableSizes() {
return get( this.props.image, [ 'data', 'media_details', 'sizes' ], {} );
}

render() {
const { attributes, setAttributes, focus, setFocus, className, settings } = this.props;
const { url, alt, caption, align, id, href, width, height } = attributes;

const availableSizes = this.getAvailableSizes();
const figureStyle = width ? { width } : {};
const availableSizes = Object.keys( this.state.availableSizes ).sort();
const selectedSize = findKey( this.state.availableSizes, ( size ) => size.source_url === url );
const isResizable = [ 'wide', 'full' ].indexOf( align ) === -1;
const uploadButtonProps = { isLarge: true };
const uploadFromFiles = ( event ) => mediaUpload( event.target.files, setAttributes );
Expand Down Expand Up @@ -181,15 +167,15 @@ class ImageBlock extends Component {
</BlockDescription>
<h3>{ __( 'Image Settings' ) }</h3>
<TextControl label={ __( 'Alternate Text' ) } value={ alt } onChange={ this.updateAlt } />
{ !! availableSizes.length && (
{ ! isEmpty( availableSizes ) && (
<SelectControl
label={ __( 'Size' ) }
value={ selectedSize || '' }
options={ availableSizes.map( ( imageSize ) => ( {
value: imageSize,
label: startCase( imageSize ),
value={ url }
options={ map( availableSizes, ( size, name ) => ( {
value: size.source_url,
label: startCase( name ),
} ) ) }
onChange={ this.updateImageSize }
onChange={ this.updateImageURL }
/>
) }
</InspectorControls>
Expand Down Expand Up @@ -257,4 +243,16 @@ class ImageBlock extends Component {
}
}

export default withEditorSettings()( ImageBlock );
export default flowRight( [
withEditorSettings(),
withAPIData( ( props ) => {
const { id } = props.attributes;
if ( ! id ) {
return {};
}

return {
image: `/wp/v2/media/${ id }`,
Copy link
Member

Choose a reason for hiding this comment

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

It's surprising that you need to combine the endpoint's url here, but otherwise it's very nice 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

It's surprising that you need to combine the endpoint's url here

Could you elaborate on what you mean by this? I'm not sure if the surprise is something I should be worried about 😄

Copy link
Member

@gziolo gziolo Oct 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I would expect to see API url creation hidden in the HOC. So in this place we could have the following instead:

return [
	id && { type: 'image', params: { id } }
];

withAPIData could filter out undefined elements to make this more conceise here.

At the moment we have the following code in a few places:

const applyWithAPIData = withAPIData( () => {
	return {
		user: '/wp/v2/users/me?context=edit',
	};
} );

which would simplify to:

const applyWithAPIData = withAPIData( () => [ { type: 'user' } ] );

withAPIData would have all mappings stored internally:
user => /wp/v2/users/me?context=edit

I might be missing something obvious here, but I'm happy to discuss if you think it would be an improvement. It's good how it is because those urls won't rather change in the future :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue I'd see is type wouldn't always be so simple. This could potentially be used for custom endpoints, which can be defined as whichever route structure the developer chooses (even /this/is/an/obnoxious/route/pattern). I noted some downsides here in the original text of #1974, including how this binds us to the REST API, but doing so in a way that is easy for developers to use (don't need to understand mappings if they already know the routes), isolated, generic.

};
} ),
] )( ImageBlock );