Add caption to gallery images #4199

jorgefilipecosta · 2017-12-28T20:32:46Z

Description

This PR adds caption to individual images in the gallery block addressing #1443

How Has This Been Tested?

Add gallery block, insert images from the media library, some with captions some without a caption. Verify that for the ones with the caption, the caption editable is pre-populated, verify it is possible to change captions.
Save the post preview and checks things look identical to what appeared in the editor.

Screenshots (jpeg or gifs if applicable):

youknowriad · 2017-12-29T09:20:40Z

blocks/library/gallery/block.js

+			this.setState( {
+				selectedImage: index,
+			} );
+			this.props.setFocus( focusValue );


I wonder how this works. the setFocus should be passed an object. Maybe something like this { index } to keep track of the focused caption.

Hi @youknowriad, normally we pass an object in the format { index: val, ... focusValue }, but in this case, we already had a mechanism to track the active image so we pass only focus value and we make use of the existing mechanism focus={ this.state.selectedImage === index && focus ? focus : undefined }. I think adding a new property to the focus value to track the active image would be redundant in this case given that we already have a mechanism for that.

Ok, so this means, if you edit the caption of the image, it's selected. (not against this) but I can't test for now as the TinyMCE CDN is broken.

Yes, that's right when we try to edit a caption we automatically select that image.

ellatrix · 2017-12-31T14:38:59Z

Somehow I was expecting all caption fields to pop up as soon as the block is selected, and empty ones to disappear again if they are empty. Currently I have to selected the gallery block (nothing happens), then select an an image (focus immediately jump to the caption, which does not happen in a normal image block), and moving selection to another block does not hide the empty caption field.

I'm also seeing the black background overlapping with the blue border (Chrome):

aduth · 2018-01-03T16:58:21Z

blocks/library/gallery/block.js

@@ -56,13 +57,23 @@ class GalleryBlock extends Component {
 	}

 	onSelectImage( index ) {
-		return () => {
+		return ( event ) => {
+			// ignore clicks in the editable caption


Why?

If it's the case that we want to avoid calling setState to the already-selected image, should we not do this more generically to test whether the index is any different than state.selectedImage ?

In either case, a more detailed comment would be helpful.

Hi @aduth, I will add a more detailed comment 👍 The reason of this check is that without it, making changes in the editable selects/un-selects the image, e.g when we try to select text to make it bold we would be selecting/unselecting many times during this process.

jorgefilipecosta · 2018-01-08T18:45:34Z

The points raised were addressed 👍

ellatrix · 2018-01-08T21:18:44Z

Looks better! 👍

The toolbar seems to be hanging a bit low:

Also, as you can see in the picture, when I focus a caption field, the image selection does not move with it.

karmatosed · 2018-01-10T11:23:51Z

There are a few visual issues we should work through before approving.

When you have images loading you get a stack of captions. Can we only show captions once the images are fully loaded?
Right now adding is a tight overlay:

Can we add more space to the toolbar and put higher, above the content?

When you are 'in' an image can we focus and not show the other captions? It's super weird seeing the same content over and over.

As a point to consider, maybe we should not show all captions by default. What about showing on just the first or showing only when you click on an image. Right now the wall of the same text visually incredibly distracting.

karmatosed

I have requested changes in comments.

jorgefilipecosta · 2018-01-12T18:27:06Z

Hi @karmatosed thank you for your design review.
Some changes were performed:

Empty caption placeholder only appears on the image that is selected e.g it appears when the image is clicked as suggested. This addresses point 3 and make visual less distracting. It also addresses point one as it stops showing empty when images are loading because they are not selected.
The position of the toolbar was corrected.
The height of the caption was increased and it is a little bit less tight.

karmatosed · 2018-01-15T11:52:56Z

I am still getting the caption appearing on all, which leaves me getting something like this:

jorgefilipecosta · 2018-01-18T21:38:00Z

Hi @karmatosed, maybe it was some caching issue or other strange intermittent problem. But with the merge of the last changes to gallery markup to render images in a list this had to suffer considerable changes in the code.
This version should only show non-empty captions of the selected image. Like on this screen:

If in your tests the design does not appear like that, please let me know and I will try to debug further.

ellatrix · 2018-01-19T10:31:52Z

blocks/library/gallery/block.js

@@ -74,7 +92,8 @@ class GalleryBlock extends Component {
 	}

 	onSelectImages( imgs ) {
-		this.props.setAttributes( { images: imgs } );
+		const images = imgs.map( img => img.caption ? { ...img, caption: [ img.caption ] } : img );
+		this.props.setAttributes( { images } );


Minor: I would probably use something more descriptive like:

onSelectImages( images ) { this.props.setAttributes( { images: images.map( ( attributes ) => ( { ...attributes, caption: attributes.caption || [], } ), } ); }

Additionally, why [ img.caption ]? Shouldn't img.caption be an array?

I followed your suggestion and used most of your code, thank you :)
Regarding this part:

Shouldn't img.caption be an array?

This function executes when the images are selected from the media library, and we receive a string from it. That's why we wrap in an array so it is ready to be used in the editable.

ellatrix · 2018-01-19T10:39:28Z

blocks/library/gallery/block.js

+		return ( event ) => {
+			// ignore clicks in the editable caption.
+			// Without this logic, text operations like selection, select / unselects the images.
+			if ( event.target.tagName === 'FIGCAPTION' ) {


What if deselecting a gallery item works similarly to selecting and deselecting blocks? So a click inside the block would not deselect the block, only a click outside it or in another block would.

I was thinking something like:

onSelectImage( index ) { return ( event ) => { this.setState( ( state ) => ( { selectedImage: index, } ) ); // unfocus currently focus editable this.props.setFocus( { ...this.props.focus, editableIndex: undefined } ); }; }

What if deselecting a gallery item works similarly to selecting and deselecting blocks? So a click inside the block would not deselect the block, only a click outside it or in another block would.

That's a nice suggestion, and I think it may be a good idea, but maybe we can do it in a different PR so we can discuss this change and receive more feedback if this is something we should pursue or no.

Well, I think this change would make the image and gallery block behave consistently and at the same time simplify the code.

ellatrix · 2018-01-19T10:40:09Z

It works really nicely now!

karmatosed · 2018-01-19T16:10:25Z

@jorgefilipecosta cache could have been the issue yes - it seems to work now thanks.

ellatrix · 2018-01-26T19:09:36Z

Seeing another small bug...

Multiline captions break.

jorgefilipecosta · 2018-02-05T15:27:30Z

Hi @iseulde, thank you for your feedback, this PR was updated and multiple line support was added.

ellatrix · 2018-02-05T18:00:22Z

I'm seeing a glitch now. When I put select an image, the caption field "jumps" for a millisecond.
In this branch images also don't immediately select when selecting the block. In master it does.
Not a blocker, but #4199 (comment) would be nice.

https://cloudup.com/cqrvOM177Yr

jorgefilipecosta · 2018-02-06T12:52:09Z

Hi @iseulde it looks like the blink in the caption was caused by a CSS rule, I used an equivalent one and things look fine now.

In this branch images also don't immediately select when selecting the block. In master it does.

They should immediately select when clicking on them even if the block was not selected like in master. I tried to find a cause for that not happening in your tests. And it looks like if developer tools are open that does not happen (also in master). Maybe a bug a in the browser.
Without developer tools:

With developer tools:

Could that have been the cause? Are you still facing this problem?

Thank you for your tests!

ellatrix · 2018-02-06T13:11:17Z

blocks/library/gallery/editor.scss

+	}
+
+	.blocks-rich-text figcaption:last-of-type {
+		position: relative;


What does this do?

I see the reason now, too bad about the rich text wrapper. Still not understanding the last-of-type though.

We want position relative, to figcaption elements in the editor except when we have a placeholder. In this cases, RichText creates 2 figcaption the first should use absolute and second relative like in the other cases. In order to not overwrite the property of the first figcaption when placeholder is visible, this rule was used. Relying on attributes like data-is-placeholder-visible caused blinks because the attributes are not immediately added to the dom. So I updated the rule and in this version, it seems there are no blinks.

Could you leave a comment?

Comment added.

ellatrix · 2018-02-06T13:15:22Z

blocks/library/gallery/editor.scss

@@ -13,6 +12,26 @@
 	&.is-transient img {
 		@include loading_fade;
 	}
+
+	.blocks-rich-text__inline-toolbar {


Should be .block-rich-text__inline-toolbar I think.

Well, it's a typo in the RichText component it seems. :)

(But this rule won't do anything.)

Yes the name was updated 👍 The spacing was also updated because right our css rules changed and we don't require a big margin.

Is it still needed at all? As long as the toolbar appears outside the black area, it's good. I didn't see it appearing inside the black area before.

Yes in this layout the toolbar is not bad even without the rule. So I removed it.

ellatrix · 2018-02-06T13:15:35Z

blocks/library/gallery/editor.scss

@@ -13,6 +12,26 @@
 	&.is-transient img {
 		@include loading_fade;
 	}
+
+	.blocks-rich-text__inline-toolbar {
+		margin-top: -1.8em;


Why change this?

Without this change, the inline toolbar is very close to the caption text this creates a space so things look better.

It doesn't look like the rule is applied though. See above.

ellatrix · 2018-02-06T13:21:47Z

They should immediately select when clicking on them even if the block was not selected like in master. I tried to find a cause for that not happening in your tests. And it looks like if developer tools are open that does not happen (also in master). Maybe a bug a in the browser.

I think it has something to do with the block scrolling into view. Anyway, if it's present in master we can open a new issue. :)

ellatrix

Works nicely. Would be nice to address the comments.

Squashed commits: [f08184b] Made gallery to image and image to gallery transformations take into consideration caption attributes. [0cf5d9e] Implemented caption editing and styles in the gallery block. [ebca968] Added caption to the attributes of images in the gallery and to the saving logic. [465b2c7] Added caption slimImageObject set of MediaUploadButton. This allows us to read to image caption from the media library in the gallery block.

jorgefilipecosta added [Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement. labels Dec 28, 2017

jorgefilipecosta self-assigned this Dec 28, 2017

jorgefilipecosta force-pushed the add/caption-gallery-images branch 3 times, most recently from aef1927 to 9fede69 Compare December 28, 2017 20:56

youknowriad added the Needs Design Feedback Needs general design feedback. label Dec 29, 2017

youknowriad reviewed Dec 29, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/caption-gallery-images branch from 9fede69 to 145252a Compare December 29, 2017 17:27

jorgefilipecosta force-pushed the add/caption-gallery-images branch from 145252a to 7736ebf Compare January 2, 2018 23:49

aduth reviewed Jan 3, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/caption-gallery-images branch from 7736ebf to f8b95d2 Compare January 8, 2018 18:29

jorgefilipecosta requested a review from karmatosed January 8, 2018 18:44

karmatosed requested changes Jan 10, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/caption-gallery-images branch from f8b95d2 to 1319c46 Compare January 12, 2018 15:38

jorgefilipecosta force-pushed the add/caption-gallery-images branch 3 times, most recently from 55d454f to 4c125b9 Compare January 18, 2018 21:32

ellatrix reviewed Jan 19, 2018

View reviewed changes

karmatosed approved these changes Jan 19, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/caption-gallery-images branch from 4c125b9 to 8374158 Compare January 25, 2018 15:26

jorgefilipecosta force-pushed the add/caption-gallery-images branch from 8374158 to 6a6996b Compare January 25, 2018 15:39

youknowriad mentioned this pull request Jan 29, 2018

Add captions to gallery block #1831

Closed

jorgefilipecosta force-pushed the add/caption-gallery-images branch from 6a6996b to 0dbe86e Compare February 5, 2018 15:25

jorgefilipecosta force-pushed the add/caption-gallery-images branch 3 times, most recently from 4a71b77 to 13388dd Compare February 6, 2018 12:31

ellatrix reviewed Feb 6, 2018

View reviewed changes

ellatrix approved these changes Feb 6, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/caption-gallery-images branch 2 times, most recently from 8f7c54a to d9d337f Compare February 6, 2018 15:02

jorgefilipecosta force-pushed the add/caption-gallery-images branch from d9d337f to c854709 Compare February 6, 2018 15:07

jorgefilipecosta merged commit ba87962 into master Feb 6, 2018

jorgefilipecosta deleted the add/caption-gallery-images branch February 6, 2018 16:34

This was referenced Feb 6, 2018

Made gallery images unselect when clicking outside the block or selecting other image. #4903

Merged

Gallery: add support for optional captions #1443

Closed

Rahmon mentioned this pull request Feb 9, 2018

Spinner at the bottom when image is loading in gallery #4973

Closed

jorgefilipecosta mentioned this pull request Feb 16, 2018

Fixed gallery link attribute selector. #5109

Merged

Add caption to gallery images #4199

Add caption to gallery images #4199

Conversation

jorgefilipecosta commented Dec 28, 2017 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

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

ellatrix commented Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta commented Jan 8, 2018

ellatrix commented Jan 8, 2018

karmatosed commented Jan 10, 2018 • edited Loading

karmatosed left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jan 12, 2018

karmatosed commented Jan 15, 2018

jorgefilipecosta commented Jan 18, 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

ellatrix commented Jan 19, 2018

karmatosed commented Jan 19, 2018

ellatrix commented Jan 26, 2018

jorgefilipecosta commented Feb 5, 2018

ellatrix commented Feb 5, 2018 • edited Loading

jorgefilipecosta commented Feb 6, 2018

Choose a reason for hiding this comment

ellatrix Feb 6, 2018 • edited Loading

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

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

jorgefilipecosta Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Feb 6, 2018

ellatrix left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Dec 28, 2017 •

edited

Loading

ellatrix commented Dec 31, 2017 •

edited

Loading

jorgefilipecosta Jan 3, 2018 •

edited

Loading

karmatosed commented Jan 10, 2018 •

edited

Loading

ellatrix commented Feb 5, 2018 •

edited

Loading

ellatrix Feb 6, 2018 •

edited

Loading

jorgefilipecosta Feb 6, 2018 •

edited

Loading