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

Try fixing captions on resized images #6496

Merged
merged 5 commits into from May 16, 2018
Merged

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Apr 30, 2018

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/
@jasmussen jasmussen self-assigned this Apr 30, 2018
@jasmussen jasmussen requested a review from WordPress/gutenberg-core Apr 30, 2018
@@ -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,

This comment has been minimized.

Copy link
@ellatrix

ellatrix Apr 30, 2018

Member

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

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

This comment has been minimized.

Copy link
@ellatrix

ellatrix Apr 30, 2018

Member

And if caption needs targeting.

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

This comment has been minimized.

Copy link
@jasmussen

jasmussen May 1, 2018

Author Contributor

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 comment has been minimized.

Copy link
@youknowriad

youknowriad May 1, 2018

Contributor

This change needs a deprecated version though :)

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 1, 2018

Member

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

This comment has been minimized.

Copy link
@jasmussen

jasmussen May 3, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

youknowriad commented Apr 30, 2018

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

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Apr 30, 2018

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:

edge

ie

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented May 1, 2018

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

screen shot 2018-05-01 at 10 27 37

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

What do you think?

display: -ms-inline-grid;
-ms-grid-columns: min-content;

> :nth-child(2) {

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 1, 2018

Member

What does this target?

This comment has been minimized.

Copy link
@jasmussen

jasmussen May 1, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@jasmussen

jasmussen May 1, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Member

karmatosed commented May 2, 2018

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

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented May 2, 2018

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

@jasmussen jasmussen added this to the 2.9 milestone May 16, 2018
@@ -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,
} );

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 16, 2018

Contributor

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.

This comment has been minimized.

Copy link
@jasmussen

jasmussen May 16, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@jasmussen

jasmussen May 16, 2018

Author Contributor

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

Copy link
Contributor

youknowriad left a comment

LGTM 👍

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented May 16, 2018

Yay, thanks!

@jasmussen jasmussen merged commit 2b5448d into master May 16, 2018
2 checks passed
2 checks passed
codecov/project 46.33% (+2.26%) compared to 1218df9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jasmussen jasmussen deleted the try/fix-caption-widths-v2 branch May 16, 2018
@@ -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, {

This comment has been minimized.

Copy link
@aduth

aduth May 23, 2018

Member

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

This comment has been minimized.

Copy link
@aduth

aduth May 23, 2018

Member

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 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.