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

Add/gallery block caption #17101

Merged
merged 23 commits into from Sep 6, 2019

Conversation

@tellthemachines
Copy link
Contributor

commented Aug 20, 2019

Description

This is an updated version of #17001 which I closed due to rebase gone wrong. I have addressed all feedback from #17001 except for @talldan's request to add an e2e test for the block as I'm not sure how to add an image to a block in an e2e test. There are no image-containing blocks being tested atm so no previous examples. Would love some input on this!

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

Editor view of gallery with caption

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.
Copy link
Member

left a comment

Left some minor comments, looks good otherwise.

packages/blocks/src/api/validation.js Outdated Show resolved Hide resolved
@@ -12,6 +12,11 @@ ul.wp-block-gallery {
}
}

// need to override default editor ul styles
.blocks-gallery-grid.blocks-gallery-grid {

This comment has been minimized.

Copy link
@ellatrix

ellatrix Aug 20, 2019

Member

Why twice?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 21, 2019

Author Contributor

I left the comment above in an attempt to clarify, but perhaps it should be more explicit. I needed to override styles that are set for all ul in the editor, here:
Screen Shot 2019-08-21 at 11 24 56 am
I like this technique of duplicating the selector to increase specificity because it makes it clear that there is no other reason for use of more than one selector. Chaining two different selectors or using a parent selector might be necessary to target a particular element, but chaining the same selector twice can only be used for specificity increase, so if we revisit this later and find that the style being overridden is gone, we'll know it's safe to remove the extra selector 🙂

This comment has been minimized.

Copy link
@ellatrix

ellatrix Aug 22, 2019

Member

But when does .blocks-gallery-grid.blocks-gallery-grid occur? This means an element will have the class twice?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 22, 2019

Author Contributor

No, the element only needs to have the class once. Chaining the same class two or more times together will only increase specificity for that rule. It'll still apply to any element that has that class on it.

{ images.map( ( image ) => {
let href;
<figure>
<ul className={ `blocks-gallery-grid columns-${ columns } ${ imageCrop ? 'is-cropped' : '' }` }>

This comment has been minimized.

Copy link
@ellatrix

ellatrix Aug 20, 2019

Member

Why add an extra class here? Why not write wp-block-gallery ul to select it?
Any reason the is-cropped class can't be moved to the parent element?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 21, 2019

Author Contributor

It's always better to target the specific element styles are being applied to with a classname. Using .className element selectors will make things messy if we ever need to add another element with a different purpose to the component. I know there are quite a few existing instances of element selectors in this codebase, but I'd like to propose we move away from that. Ideally, every element that requires CSS should have its own classname; it'll save us a lot of grief in the long run.

No reason is-cropped can't move; I left it there along with the other gallery-related classes like columns and align, but now I see align at least should be applied to the outer wrapper so might as well move the whole lot.

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

This comment has been minimized.

Copy link
@ellatrix

ellatrix Aug 20, 2019

Member

This seems a bit strange to me as it will duplicate all the nested rules, even the ones targeting child elements. Could it be separated with only the necessary rules added?

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Aug 21, 2019

Author Contributor

If I remove all the styles from wp-block-gallery it breaks the deprecated instances of the block. Not sure if there's any other way around this issue.

packages/block-library/src/gallery/theme.scss Outdated Show resolved Hide resolved
packages/block-library/src/gallery/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/gallery/save.js Outdated Show resolved Hide resolved
packages/block-library/src/gallery/save.js Outdated Show resolved Hide resolved
@ellatrix

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

There's an e2e test failure with the demo content.

packages/block-library/src/gallery/block.json Outdated Show resolved Hide resolved
post-content.php Show resolved Hide resolved
@tellthemachines tellthemachines force-pushed the add/gallery-block-caption branch from 79555cb to 2d43726 Sep 3, 2019
@tellthemachines tellthemachines requested review from talldan and ellatrix Sep 4, 2019
Copy link
Contributor

left a comment

Looks good, just that question about galleryCaptionFocused that it'd be good to understand more about.

@@ -238,6 +249,11 @@ class GalleryEdit extends Component {
captionSelected: false,
} );
}
if ( ! this.props.isSelected && prevProps.isSelected && this.state.galleryCaptionFocused ) {
this.setState( {
galleryCaptionFocused: false,

This comment has been minimized.

Copy link
@talldan

talldan Sep 4, 2019

Contributor

It looks like some things have been refactored in this pr and I now can't see where galleryCaptionFocused is ever set to true. Maybe it and reference to it can be removed.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 5, 2019

Author Contributor

Oh, well spotted. Removed the state altogether.

value={ caption }
unstableOnFocus={ this.onFocusGalleryCaption }
onChange={ ( value ) => setAttributes( { caption: value } ) }
isSelected={ this.state.galleryCaptionFocused }

This comment has been minimized.

Copy link
@talldan

talldan Sep 4, 2019

Contributor

Related to the previous comment, I wonder how this could ever be true.

@tellthemachines tellthemachines requested a review from talldan Sep 5, 2019
@talldan
talldan approved these changes Sep 6, 2019
Copy link
Contributor

left a comment

I know this one wasn't straightforward, great work here @tellthemachines!

@tellthemachines tellthemachines merged commit c861f94 into master Sep 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@tellthemachines tellthemachines deleted the add/gallery-block-caption branch Sep 6, 2019
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
@kwight

This comment has been minimized.

Copy link

commented Sep 25, 2019

It looks like block alignment might not have been handled by this PR: #17584

dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* Basic gallery caption.

* add block deprecation

* Regenerate test fixtures and fix failures

* e2e snapshot update

* Fix editor styles

* Fix block transforms e2e

* Workaround for failing tests

* Undo package-lock changes

* Revert "Undo package-lock changes"

This reverts commit f4f8ba9.

* Fix messed up styles.

* Try to fix package-lock issue _again_

* Addressed design feedback.

* Addressed previous PR feedback

* Addressed latest PR feedback

* Update post content and improve selectors

* Change selectors for captions.

* Change selectors and focus behaviour.

* Update fixture.

* Update demo post content again.

* Update e2e snapshots

* Redo snapshots again.

* Change conditional hiding on empty/unselected

* Remove unnecessary local state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.