Add/caption for gallery block #17001

tellthemachines · 2019-08-12T04:33:47Z

Description

Added a caption for the gallery block. This caption refers to the gallery as a whole, so semantically it works by wrapping the whole gallery block in a <figure> element and adding a <figcaption> with the caption inside.

How has this been tested?

Tested locally, checked if previously created galleries still work, ran unit tests, updated fixtures.

Note: The caption on the editor side will likely have the same accessibility issues as the image caption. These are discussed in detail in @talldan's table caption PR
I don't believe we have agreed on the best way forward for that, so it might be best to address it as a separate PR.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

gziolo · 2019-08-12T10:34:51Z

package-lock.json

@@ -7708,7 +7708,7 @@
 							"dependencies": {
 								"minimist": {
 									"version": "1.2.0",
-									"resolved": false,
+									"resolved": "",


This is surprising to see those changes.

Oh bugger, shouldn't have committed this file. I keep getting this diff when I run npm install even though I'm updating my npm version regularly 😩

So reverting this file to its previous state before my commit doesn't work because it's been updated in master in the meantime. I tried nvm install --latest-npm followed by npm install and even npm run docs:build on my local branch and there's no diff. Is it possible the current package-lock on master wasn't generated with the latest npm?

Yes, if there is new npm version, and they changed something then we will have to patch master.

gziolo · 2019-08-12T10:35:51Z

packages/block-library/src/gallery/block.json

@@ -48,6 +48,11 @@
 		"columns": {
 			"type": "number"
 		},
+		"galleryCaption": {


Can it be caption? It seems repetitive to include gallery as a prefix.

The problem is that gallery images already have captions so it's less confusing if this one has a name that clearly indicates what it is a caption of.

It depends on where you look at, I found it confusing when seeing it in the attributes section block.json file. Gallery itself can have only one caption. In the future, we might refactor this block and convert all images to nested blocks at the issue you pointed out would solve on its own. You can always alias the caption of an image if it has to be referenced in the same scope or extract the image logic to its own component.

gziolo · 2019-08-12T10:40:14Z

packages/block-library/src/gallery/save.js

+		</>
+	);
+
+	if ( galleryCaption ) {


If there is no change in the markup for the case when we don't provide the caption, do we really need to build all deprecation story? It seems like it should out of the box if the caption is not provided. I don't see any changes in the deprecated version serialized in the fixtures folder (input is almost identical to the output when you ignore whitespaces).

I'm not sure! The markup of the new version is the same but there's an added attribute. Happy to remove the deprecation if it's not needed though 🙂

You would have to tests it but I know we had similar cases in the past and we didn't have to build deprecation logic when new optional HTML attributes were added or wrapped with optional HTML tags. It seems like this is the case here.

gziolo · 2019-08-12T10:45:58Z

packages/block-library/src/gallery/style.scss

@@ -1,8 +1,10 @@
-.wp-block-gallery {
+.wp-block-gallery,


In the case where Gallery block has a caption then it produces HTML where .blocks-gallery-grid is enclosed in .wp-block-gallery, won't it cause any issues?

I've tested this and added an override on the editor side for figure.wp-block-gallery to display:block as the flex display was causing issues. It seems to be fine on the post rendering side so far, but I can always add that override globally if needed. The other styles are pretty harmless.

gziolo · 2019-08-12T10:48:00Z

packages/block-library/src/gallery/style.scss

@@ -147,4 +149,11 @@
 			justify-content: center;
 		}
 	}
+
+	+ figcaption {


Where those styles should get applied? To the gallery's caption only? If yes, then it should be placed outside of .blocks-gallery-grid.

Yup these are for the gallery caption only, and the problem here is that the RichText component won't allow me to add a classname to that element. Because there can potentially be other figcaptions inside the gallery, I'm targeting this one as .blocks-gallery-grid + figcaption. The plus sign between the two selectors makes it target only the figcaption that is the immediate next sibling to .blocks-gallery-grid.

This reverts commit f4f8ba9.

* Update github action exit codes * Remove Filter Opened from github actions

* Extract caption component * Use caption component for video * Move Caption to block-editor for mobile tests This move avoids a dependency issue that was causing the mobile tests to fail due to the Caption component's import of RichText from the block-editor package. * Pass onBlur to Caption component * Connect Caption component to store

…untitled posts (#17074)

This reverts commit 9a0876c.

…gutenberg into add/caption-for-gallery-block

tellthemachines · 2019-08-19T08:52:26Z

Closing this PR because I messed up the rebase, will have updated one up soon.

tellthemachines marked this pull request as ready for review August 12, 2019 09:19

tellthemachines requested review from ajitbohra, ellatrix, gziolo, jorgefilipecosta, nerrad, ntwb, Soean, talldan and youknowriad as code owners August 12, 2019 09:19

gziolo reviewed Aug 12, 2019

View reviewed changes

gziolo added [Block] Gallery Affects the Gallery Block - used to display groups of images [Type] Enhancement A suggestion for improvement. labels Aug 12, 2019

tellthemachines added 7 commits August 13, 2019 10:47

Basic gallery caption.

6465790

add block deprecation

9fc6699

Regenerate test fixtures and fix failures

f635ed9

e2e snapshot update

3f90fa4

Fix editor styles

9751b39

Fix block transforms e2e

bbaf7a3

Workaround for failing tests

4a79b86

tellthemachines force-pushed the add/caption-for-gallery-block branch from 1d0d568 to 4a79b86 Compare August 13, 2019 00:47

tellthemachines added 2 commits August 13, 2019 10:51

Undo package-lock changes

f4f8ba9

Revert "Undo package-lock changes"

321d469

This reverts commit f4f8ba9.

tellthemachines added the Needs Design Feedback Needs general design feedback. label Aug 13, 2019

talldan and others added 3 commits August 13, 2019 12:16

Update github action exit codes (#17002)

18b194a

* Update github action exit codes * Remove Filter Opened from github actions

Fix messed up styles.

f6acb42

Try to fix package-lock issue _again_

9aa06d3

gziolo requested a review from jasmussen August 13, 2019 07:35

jorgefilipecosta and others added 19 commits August 16, 2019 15:12

Fix: Button block does not centers on the editor (#17063)

fbf1938

Update: Latest Posts Block: Use (no title) instead of (Untitled) for …

d80b024

…untitled posts (#17074)

Revert "Change caption name and remove deprecation."

82fcf2b

This reverts commit 9a0876c.

Re-add gallery caption name change.

7b06d77

Basic gallery caption.

b5bea97

add block deprecation

647adf0

Regenerate test fixtures and fix failures

752c993

e2e snapshot update

40ada1d

Fix editor styles

09a571d

Fix block transforms e2e

1fe3d2e

Workaround for failing tests

2572f36

Undo package-lock changes

b978a02

Fix messed up styles.

220968e

Addressed design feedback.

02171ed

Change caption name and remove deprecation.

1302a85

Revert "Change caption name and remove deprecation."

1361712

This reverts commit 9a0876c.

Re-add gallery caption name change.

c401419

Merge branch 'add/caption-for-gallery-block' of github.com:WordPress/…

5e65169

…gutenberg into add/caption-for-gallery-block

tellthemachines requested review from chrisvanpatten, daniloercoli, etoledom, karmatosed, mapk, mkaz and SergioEstevao as code owners August 19, 2019 08:39

tellthemachines closed this Aug 19, 2019

youknowriad deleted the add/caption-for-gallery-block branch August 19, 2019 09:34

tellthemachines mentioned this pull request Aug 20, 2019

Add/gallery block caption #17101

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/caption for gallery block #17001

Add/caption for gallery block #17001

tellthemachines commented Aug 12, 2019 •

edited

Loading

gziolo Aug 12, 2019

tellthemachines Aug 13, 2019

tellthemachines Aug 13, 2019

gziolo Aug 13, 2019

gziolo Aug 12, 2019

tellthemachines Aug 13, 2019

gziolo Aug 13, 2019

gziolo Aug 12, 2019

tellthemachines Aug 13, 2019

gziolo Aug 13, 2019

gziolo Aug 12, 2019

tellthemachines Aug 13, 2019

gziolo Aug 12, 2019

tellthemachines Aug 13, 2019

tellthemachines commented Aug 19, 2019

		@@ -1,8 +1,10 @@
		.wp-block-gallery {
		.wp-block-gallery,

Add/caption for gallery block #17001

Add/caption for gallery block #17001

Conversation

tellthemachines commented Aug 12, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines commented Aug 19, 2019

tellthemachines commented Aug 12, 2019 •

edited

Loading