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

Try fixing captions on resized images #6496

Merged
merged 5 commits into from
May 16, 2018
Merged

Try fixing captions on resized images #6496

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26a61aa
Try fixing captions on resized images
Apr 30, 2018
779f439
Add fix for Edge and IE11
May 1, 2018
6f3107b
Polish up Edge min-content trick.
jasmussen May 1, 2018
68b40e5
Add deprecated version.
jasmussen May 3, 2018
c060ca9
Fix deprecated version
youknowriad May 16, 2018
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
16 changes: 14 additions & 2 deletions core-blocks/image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -165,6 +170,10 @@ export const settings = {
save( { attributes } ) {
const { url, alt, caption, align, href, width, height, id } = attributes;

const classes = classnames( align ? `align${ align }` : null, {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Good use case for classnames object format is to inverse this, so you don't need to assign a fallback null value to be omitted:

const classes = classnames( {
	[ `align${ align }` ]: align,
	// ...
} );

'is-resized': !! width || !! height,
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, could we remove the need for this class with the following?

img[height],
img[width] {
  ...
}

Copy link
Member

Choose a reason for hiding this comment

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

And if caption needs targeting.

img[height] + figcaption,
img[width] + figcaption {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, we cannot, because width: min-content has to be applied to the parent container, in this case the figure. I suppose we could it as a data-is-resized attribute instead, and target that, if we want to get rid of classes. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

This change needs a deprecated version though :)

Copy link
Member

Choose a reason for hiding this comment

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

^ Yeah this is why I was wondering if we could avoid introducing new attributes, but ok if really needed! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a deprecated version in 68b40e5. Did I do that right?

To clarify what happens — if the figure is as wide as the main content, it needs fit-content. If the figure is smaller than the main content, it needs min-content so that a long caption doesn't make the figure wider than its smallest element (which will always be the image).

Should the class be called "is-resized" for that, or can we think of something better?

Copy link
Member

Choose a reason for hiding this comment

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

Minor: classnames doesn't require the value to be explicitly a boolean, so this could have been just width || height. Or, if a boolean is desired, coerce after the fact: Boolean( width || height )

} );

const image = (
<img
src={ url }
Expand All @@ -176,7 +185,7 @@ export const settings = {
);

return (
<figure className={ align ? `align${ align }` : null }>
<figure className={ classes }>
{ href ? <a href={ href }>{ image }</a> : image }
{ caption && caption.length > 0 && <RichText.Content tagName="figcaption" value={ caption } /> }
</figure>
Expand All @@ -190,6 +199,9 @@ export const settings = {
const { url, alt, caption, align, href, width, height } = attributes;
const extraImageProps = width || height ? { width, height } : {};
const image = <img src={ url } alt={ alt } { ...extraImageProps } />;
const classes = classnames( align ? `align${ align }` : null, {
'is-resized': !! width || !! height,
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change? I think the deprecated versions shouldn't be updated once they land. I think what we need to do here is to add a new deprecated version with the save function without the is-resized className. That way the blocks using that version would be considered as valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Can you help me with this? Feel free to push directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Can you help me with this? Feel free to push directly


let figureStyle = {};

Expand All @@ -200,7 +212,7 @@ export const settings = {
}

return (
<figure className={ align ? `align${ align }` : null } style={ figureStyle }>
<figure className={ classes } style={ figureStyle }>
{ href ? <a href={ href }>{ image }</a> : image }
{ caption && caption.length > 0 && <RichText.Content tagName="figcaption" value={ caption } /> }
</figure>
Expand Down
17 changes: 17 additions & 0 deletions core-blocks/image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.wp-block-image {
width: fit-content;

figcaption {
margin-top: 0.5em;
color: $dark-gray-300;
Expand All @@ -13,4 +14,20 @@
margin-right: auto;
text-align: center;
}

&.is-resized {
width: min-content;

// Emulate min-content for Edge and IE11
display: -ms-inline-grid;
-ms-grid-columns: min-content;

figcaption {
-ms-grid-row: 2;
}

img {
max-width: none;
}
}
}