Try fixing captions on resized images #6496

jasmussen · 2018-04-30T09:31:15Z

This is an alternative, and probably better approach, than the one explored in #6304.

Both branches try and fix the same issue — ensuring that the caption inside a figure is only ever as wide as the adjecent image. This is only ah issue when that image is resized.

This branch fixes that, in a fairly clean way. See the details in https://css-tricks.com/almanac/properties/w/width/

The figure now uses both fit-content, and min-content, both rather magical. Fit-content uses the maximum width available, and is applied to images that are not resized. Min-content uses the minimum width available, and is applied when images are resized. In effect this means that the image in a figure with a caption will define the width of the caption itself.

This is an alternative, and probably better approach, than the one explored in #6304. Both branches try and fix the same issue — ensuring that the caption inside a `figure` is only ever as wide as the adjecent image. This is only ah issue when that image is resized. This branch fixes that, in a fairly clean way. See the details in https://css-tricks.com/almanac/properties/w/width/

ellatrix · 2018-04-30T12:10:29Z

core-blocks/image/index.js

@@ -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, {
+			'is-resized': !! width || !! height,


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

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

And if caption needs targeting.

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

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?

This change needs a deprecated version though :)

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

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?

youknowriad · 2018-04-30T12:12:20Z

Apparently, fit-content and min-content don't work on IE11 and Edge, Maybe we can't rely on them ☹️

jasmussen · 2018-04-30T12:44:49Z

Bummer about that browser support. However I may have found a solution.

This codepen works for me in all the good browsers, and also Edge and IE: https://codepen.io/joen/pen/pVevQo?editors=1100

Key is that the image height is set explicitly, or it'll look weird. But it seems like this should work for us? Haven't tried floating it yet, but it should work.

Screenshots:

jasmussen · 2018-05-01T08:31:15Z

Pushed the previously mentioned fix for IE11 and Edge. It seems to be working:

I'm getting a general JS error in IE11 in the editor, but that seems unrelated to this PR.

What do you think?

ellatrix · 2018-05-01T11:24:17Z

core-blocks/image/style.scss

+		display: -ms-inline-grid;
+		-ms-grid-columns: min-content;
+
+		> :nth-child(2) {


What does this target?

I'm actually not 100% sure, it's part of the edge trick. But I'm wondering if maybe it's supposed to target the figcaption, in which case that would be better to write. I'll do some testing, see if we can get away with something cleaner.

Great catch — this was supposed to target everything after the first image, but since there's only ever a figcaption, it's much cleaner to do just that, and the inline-block was unnecessary also.

karmatosed · 2018-05-02T09:51:05Z

Visually 👍 on this. I think it's a good fix for this issue.

ellatrix · 2018-05-02T14:54:57Z

@jasmussen Code-wise, this still need to be done as the HTML output is different: https://github.com/WordPress/gutenberg/pull/6496/files#r185195135.

youknowriad · 2018-05-16T11:06:50Z

core-blocks/image/index.js

@@ -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,
+				} );


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.

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

youknowriad

LGTM 👍

jasmussen · 2018-05-16T11:51:56Z

Yay, thanks!

aduth · 2018-05-23T13:53:26Z

core-blocks/image/index.js

@@ -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, {


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, // ... } );

aduth · 2018-05-23T13:54:14Z

core-blocks/image/index.js

@@ -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, {
+			'is-resized': !! width || !! height,


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 )

jasmussen added the [Type] Enhancement A suggestion for improvement. label Apr 30, 2018

jasmussen self-assigned this Apr 30, 2018

jasmussen requested a review from a team April 30, 2018 09:31

jasmussen mentioned this pull request Apr 30, 2018

Try: Fix caption widths using CSS variable #6304

Closed

ellatrix reviewed Apr 30, 2018

View reviewed changes

Add fix for Edge and IE11

779f439

ellatrix reviewed May 1, 2018

View reviewed changes

Polish up Edge min-content trick.

6f3107b

Add deprecated version.

68b40e5

jasmussen added this to the 2.9 milestone May 16, 2018

youknowriad reviewed May 16, 2018

View reviewed changes

Fix deprecated version

c060ca9

youknowriad approved these changes May 16, 2018

View reviewed changes

jasmussen merged commit 2b5448d into master May 16, 2018

jasmussen deleted the try/fix-caption-widths-v2 branch May 16, 2018 11:52

aduth reviewed May 23, 2018

View reviewed changes

jasmussen mentioned this pull request May 24, 2018

Address Andrew feedback #6930

Merged

jasmussen mentioned this pull request Jul 4, 2018

Move "opinionated" Gutenberg block styles to theme.scss #7624

Merged

MatzeKitt mentioned this pull request Aug 13, 2018

Fixing captions on resized image broke responsiveness #8931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try fixing captions on resized images #6496

Try fixing captions on resized images #6496

jasmussen commented Apr 30, 2018

ellatrix Apr 30, 2018

ellatrix Apr 30, 2018

jasmussen May 1, 2018

youknowriad May 1, 2018

ellatrix May 1, 2018

jasmussen May 3, 2018

youknowriad commented Apr 30, 2018 •

edited

Loading

jasmussen commented Apr 30, 2018

jasmussen commented May 1, 2018

ellatrix May 1, 2018

jasmussen May 1, 2018

jasmussen May 1, 2018

karmatosed commented May 2, 2018

ellatrix commented May 2, 2018

youknowriad May 16, 2018

jasmussen May 16, 2018

jasmussen May 16, 2018

youknowriad left a comment

jasmussen commented May 16, 2018

aduth May 23, 2018

aduth May 23, 2018

Try fixing captions on resized images #6496

Try fixing captions on resized images #6496

Conversation

jasmussen commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Apr 30, 2018 • edited Loading

jasmussen commented Apr 30, 2018

jasmussen commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmatosed commented May 2, 2018

ellatrix commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

jasmussen commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Apr 30, 2018 •

edited

Loading