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

Media & Text Block: Add image fill option #14445

Merged
Merged
@@ -6,6 +6,7 @@
- Add `wide` and `full` alignments to Categories block ([#14533](https://github.com/WordPress/gutenberg/pull/14533)).
- Add all alignment options to RSS block ([#14533](https://github.com/WordPress/gutenberg/pull/14533)).
- Add all alignment options to Search block ([#14533](https://github.com/WordPress/gutenberg/pull/14533)).
- Add image fill option and focal point picker to Media & Text block ([#14445](https://github.com/WordPress/gutenberg/pull/14445)).
- Updated the edit flow of the `image` block, updated the edit icon and unified the image editing in only one UI based on `MediaPlaceholder`

### Bug Fixes
@@ -0,0 +1,107 @@
/**

This comment has been minimized.

Copy link
@frontdevde

frontdevde Mar 14, 2019

Author Contributor

Moved the deprecated part to its own file to declutter index.js.

* External dependencies
*/
import classnames from 'classnames';
import { noop } from 'lodash';

/**
* WordPress dependencies
*/
import {
InnerBlocks,
getColorClassName,
} from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import { DEFAULT_MEDIA_WIDTH } from './index';

export default [
{
This conversation was marked as resolved by frontdevde

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2019

Member

Hi @frontdevde,
I'm not understanding why this PR requires the addition of a deprecation, I'm probably missing something. If imageFill is unset the output of the block is exactly the same as before right? If that's the case I think the deprecation is not needed. I did some tests and creating blocks in the version on master and they seem valid on this branch even without the deprecation.

This comment has been minimized.

Copy link
@frontdevde

frontdevde Apr 2, 2019

Author Contributor

The deprecation was introduced at an earlier stage of the PR where it was needed. With the most recent changes to the PR, it might not be needed anymore. Thanks for pointing this out, I'll double check and address it accordingly.

This comment has been minimized.

Copy link
@frontdevde

frontdevde Apr 3, 2019

Author Contributor

You were right, just pushed a commit that removes the deprecation. Good catch!

attributes: {
align: {
type: 'string',
default: 'wide',
},
backgroundColor: {
type: 'string',
},
customBackgroundColor: {
type: 'string',
},
mediaAlt: {
type: 'string',
source: 'attribute',
selector: 'figure img',
attribute: 'alt',
default: '',
},
mediaPosition: {
type: 'string',
default: 'left',
},
mediaId: {
type: 'number',
},
mediaUrl: {
type: 'string',
source: 'attribute',
selector: 'figure video,figure img',
attribute: 'src',
},
mediaType: {
type: 'string',
},
mediaWidth: {
type: 'number',
default: 50,
},
isStackedOnMobile: {
type: 'boolean',
default: false,
},
},
save( { attributes } ) {
const {
backgroundColor,
customBackgroundColor,
isStackedOnMobile,
mediaAlt,
mediaPosition,
mediaType,
mediaUrl,
mediaWidth,
} = attributes;
const mediaTypeRenders = {
image: () => <img src={ mediaUrl } alt={ mediaAlt } />,
video: () => <video controls src={ mediaUrl } />,
};
const backgroundClass = getColorClassName( 'background-color', backgroundColor );
const className = classnames( {
'has-media-on-the-right': 'right' === mediaPosition,
[ backgroundClass ]: backgroundClass,
'is-stacked-on-mobile': isStackedOnMobile,
} );

let gridTemplateColumns;
if ( mediaWidth !== DEFAULT_MEDIA_WIDTH ) {
gridTemplateColumns = 'right' === mediaPosition ? `auto ${ mediaWidth }%` : `${ mediaWidth }% auto`;
}
const style = {
backgroundColor: backgroundClass ? undefined : customBackgroundColor,
gridTemplateColumns,
};
return (
<div className={ className } style={ style }>
<figure className="wp-block-media-text__media" >
{ ( mediaTypeRenders[ mediaType ] || noop )() }
</figure>
<div className="wp-block-media-text__content">
<InnerBlocks.Content />
</div>
</div>
);
},
},
];
@@ -23,6 +23,7 @@ import {
ToggleControl,
Toolbar,
ExternalLink,
FocalPointPicker,
} from '@wordpress/components';
/**
* Internal dependencies
@@ -77,6 +78,8 @@ class MediaTextEdit extends Component {
mediaId: media.id,
mediaType,
mediaUrl: src || media.url,
imageFill: undefined,
focalPoint: undefined,
} );
}

@@ -99,15 +102,15 @@ class MediaTextEdit extends Component {

renderMediaArea() {
const { attributes } = this.props;
const { mediaAlt, mediaId, mediaPosition, mediaType, mediaUrl, mediaWidth } = attributes;
const { mediaAlt, mediaId, mediaPosition, mediaType, mediaUrl, mediaWidth, imageFill, focalPoint } = attributes;

return (
<MediaContainer
className="block-library-media-text__media-container"
onSelectMedia={ this.onSelectMedia }
onWidthChange={ this.onWidthChange }
commitWidthChange={ this.commitWidthChange }
{ ...{ mediaAlt, mediaId, mediaType, mediaUrl, mediaPosition, mediaWidth } }
{ ...{ mediaAlt, mediaId, mediaType, mediaUrl, mediaPosition, mediaWidth, imageFill, focalPoint } }
/>
);
}
@@ -128,6 +131,9 @@ class MediaTextEdit extends Component {
mediaType,
mediaWidth,
verticalAlignment,
mediaUrl,
imageFill,
focalPoint,
} = attributes;
const temporaryMediaWidth = this.state.mediaWidth;
const classNames = classnames( className, {
@@ -136,6 +142,7 @@ class MediaTextEdit extends Component {
[ backgroundColor.class ]: backgroundColor.class,
'is-stacked-on-mobile': isStackedOnMobile,
[ `is-vertically-aligned-${ verticalAlignment }` ]: verticalAlignment,
'is-image-fill': imageFill,
} );
const widthString = `${ temporaryMediaWidth || mediaWidth }%`;
const style = {
@@ -173,6 +180,19 @@ class MediaTextEdit extends Component {
isStackedOnMobile: ! isStackedOnMobile,
} ) }
/>
{ mediaType === 'image' && ( <ToggleControl
label={ __( 'Crop image to fill entire column' ) }
checked={ imageFill }
onChange={ () => setAttributes( {
imageFill: ! imageFill,
} ) }
/> ) }
{ imageFill && ( <FocalPointPicker
label={ __( 'Focal Point Picker' ) }
url={ mediaUrl }
value={ focalPoint }
onChange={ ( value ) => setAttributes( { focalPoint: value } ) }
/> ) }
{ mediaType === 'image' && ( <TextareaControl
label={ __( 'Alt Text (Alternative Text)' ) }
value={ mediaAlt }
@@ -22,6 +22,11 @@
width: 100% !important;
}

.wp-block-media-text.is-image-fill .editor-media-container__resizer {
// The resizer sets an inline height but for the image fill we set it to full height.
height: 100% !important;
}

.wp-block-media-text .block-editor-inner-blocks {
word-break: break-word;
grid-area: media-text-content;
@@ -19,8 +19,10 @@ import { __ } from '@wordpress/i18n';
*/
import edit from './edit';
import icon from './icon';
import deprecated from './deprecated';
import { imageFillStyles } from './media-container';

const DEFAULT_MEDIA_WIDTH = 50;
export const DEFAULT_MEDIA_WIDTH = 50;

export const name = 'core/media-text';

@@ -69,6 +71,12 @@ const blockAttributes = {
verticalAlignment: {
type: 'string',
},
imageFill: {
This conversation was marked as resolved by frontdevde

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 27, 2019

Member

We need to reset these two attributes when the user selects a video as the media element, after having first selected an image. Currently, we have a bug if the user selects an image and then changes to a video the focal point picker continues to appear (without any image there).

Related question: Would the fill mode also make sense for videos? Should it just be called fill to allow a future addition?

This comment has been minimized.

Copy link
@frontdevde

frontdevde Mar 29, 2019

Author Contributor

Good catch, added a reset for the two attributes!

type: 'boolean',
},
focalPoint: {
type: 'object',
},
};

export const settings = {
@@ -160,6 +168,8 @@ export const settings = {
mediaWidth,
mediaId,
verticalAlignment,
imageFill,
focalPoint,
} = attributes;
const mediaTypeRenders = {
image: () => <img src={ mediaUrl } alt={ mediaAlt } className={ ( mediaId && mediaType === 'image' ) ? `wp-image-${ mediaId }` : null } />,
@@ -171,7 +181,9 @@ export const settings = {
[ backgroundClass ]: backgroundClass,
'is-stacked-on-mobile': isStackedOnMobile,
[ `is-vertically-aligned-${ verticalAlignment }` ]: verticalAlignment,
'is-image-fill': imageFill,
} );
const backgroundStyles = imageFill ? imageFillStyles( mediaUrl, focalPoint ) : {};
This conversation was marked as resolved by frontdevde

This comment has been minimized.

Copy link
@getdave

getdave Apr 1, 2019

Contributor

I noticed the focal point picker doesn't have initial values populated on first render (ie: before you click to select any point on the image). Is that intentional?

When testing with the cover Block, I see there are default values applied so it makes me wonder if we're doing it differently for a reason:

Screen Shot 2019-04-01 at 09 36 03

This comment has been minimized.

Copy link
@frontdevde

frontdevde Apr 1, 2019

Author Contributor

Just pushed an update that fixes an issue with the attribute resets.


let gridTemplateColumns;
if ( mediaWidth !== DEFAULT_MEDIA_WIDTH ) {
@@ -183,7 +195,7 @@ export const settings = {
};
return (
<div className={ className } style={ style }>
<figure className="wp-block-media-text__media" >
<figure className="wp-block-media-text__media" style={ backgroundStyles }>
{ ( mediaTypeRenders[ mediaType ] || noop )() }
</figure>
<div className="wp-block-media-text__content">
@@ -193,50 +205,5 @@ export const settings = {
);
},

deprecated: [
{
attributes: blockAttributes,
save( { attributes } ) {
const {
backgroundColor,
customBackgroundColor,
isStackedOnMobile,
mediaAlt,
mediaPosition,
mediaType,
mediaUrl,
mediaWidth,
} = attributes;
const mediaTypeRenders = {
image: () => <img src={ mediaUrl } alt={ mediaAlt } />,
video: () => <video controls src={ mediaUrl } />,
};
const backgroundClass = getColorClassName( 'background-color', backgroundColor );
const className = classnames( {
'has-media-on-the-right': 'right' === mediaPosition,
[ backgroundClass ]: backgroundClass,
'is-stacked-on-mobile': isStackedOnMobile,
} );

let gridTemplateColumns;
if ( mediaWidth !== DEFAULT_MEDIA_WIDTH ) {
gridTemplateColumns = 'right' === mediaPosition ? `auto ${ mediaWidth }%` : `${ mediaWidth }% auto`;
}
const style = {
backgroundColor: backgroundClass ? undefined : customBackgroundColor,
gridTemplateColumns,
};
return (
<div className={ className } style={ style }>
<figure className="wp-block-media-text__media" >
{ ( mediaTypeRenders[ mediaType ] || noop )() }
</figure>
<div className="wp-block-media-text__content">
<InnerBlocks.Content />
</div>
</div>
);
},
},
],
deprecated,
};
@@ -21,6 +21,15 @@ import icon from './media-container-icon';
*/
const ALLOWED_MEDIA_TYPES = [ 'image', 'video' ];

export function imageFillStyles( url, focalPoint ) {
return url ?
{
backgroundImage: `url(${ url })`,
backgroundPosition: focalPoint ? `${ focalPoint.x * 100 }% ${ focalPoint.y * 100 }%` : `initial`,

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 2, 2019

Member

I guess here we can just not set the property when focalPoint is undefined to be IE compatible?

} :
{};
}

class MediaContainer extends Component {
renderToolbarEditButton() {
const { mediaId, onSelectMedia } = this.props;
@@ -46,11 +55,12 @@ class MediaContainer extends Component {
}

renderImage() {
const { mediaAlt, mediaUrl, className } = this.props;
const { mediaAlt, mediaUrl, className, imageFill, focalPoint } = this.props;
const backgroundStyles = imageFill ? imageFillStyles( mediaUrl, focalPoint ) : {};
return (
<Fragment>
{ this.renderToolbarEditButton() }
<figure className={ className }>
<figure className={ className } style={ backgroundStyles }>
<img src={ mediaUrl } alt={ mediaAlt } />
This conversation was marked as resolved by frontdevde

This comment has been minimized.

Copy link
@getdave

getdave Apr 1, 2019

Contributor

Despite what I said previously about the image always needing to be available, I wonder whether, when not alt text is provided, we should not include the img at all.

The alt text box in Gutenberg UI says

Describe the purpose of the image(opens in a new tab)Leave empty if the image is purely decorative.

Purely decorative images do not need to be described in HTML as per this from the W3C

Decorative images don’t add information to the content of a page. For example, the information provided by the image might already be given using adjacent text, or the image might be included to make the website more visually attractive. In these cases, a null (empty) alt text should be provided (alt="") so that they can be ignored by assistive technologies, such as screen readers

This is a super minor point, but it might be worth considering that if the user has not entered alt text we can assume the image is purely decorative and therefore not include the img at all. Whereas if alt text is provided we need to ensure the img tag is available.

This comment has been minimized.

Copy link
@frontdevde

frontdevde Apr 1, 2019

Author Contributor

This is something we might want to consider. Given that this is behavior that predates this PR, in the interest of moving this PR along, I'd suggest that we might want to solve this in another PR. For now, I'll mark this conversation as resolved and we can open a separate issue for this.

</figure>
</Fragment>
@@ -41,6 +41,24 @@
vertical-align: middle;
}

.wp-block-media-text.is-image-fill figure {
height: 100%;
min-height: 250px;
background-size: cover;
}

.wp-block-media-text.is-image-fill figure > img {
// The image is visually hidden but accessible to assistive technologies.
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
border: 0;
}

/*
* Here we here not able to use a mobile first CSS approach.
* Custom widths are set using inline styles, and on mobile,
@@ -0,0 +1,13 @@
<!-- wp:media-text {"mediaId":11,"mediaType":"image","imageFill":true} -->
This conversation was marked as resolved by frontdevde

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 3, 2019

Member

As of today, we are using, the fixtures as part of the transforms tests to media blocks. This means we can not use URLs of images that trigger 404 errors or media ids that don't exist in the test server. The easiest solution is using a data URL. We also need to update the transforms tests structure and specify this fixture can be transformed into an image. In PR #14790 we apply all the required actions to other fixtures.

<div class="wp-block-media-text alignwide is-image-fill">
<figure class="wp-block-media-text__media"
style="background-image:url(http://localhost:8888/wp-content/uploads/2019/03/Dzts-v2X0AMxju_-1024x1024.jpg);background-position:initial">
<img src="http://localhost:8888/wp-content/uploads/2019/03/Dzts-v2X0AMxju_-1024x1024.jpg" alt="My alt text"
class="wp-image-11" /></figure>
<div class="wp-block-media-text__content">
<!-- wp:paragraph {"placeholder":"Content…","fontSize":"large"} -->
<p class="has-large-font-size">My Content</p>
<!-- /wp:paragraph -->
</div>
</div>
<!-- /wp:media-text -->
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.