Conversation
5da8501 to
078af70
Compare
ellatrix
left a comment
There was a problem hiding this comment.
Left some minor comments, looks good otherwise.
| } | ||
|
|
||
| // need to override default editor ul styles | ||
| .blocks-gallery-grid.blocks-gallery-grid { |
There was a problem hiding this comment.
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:
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 🙂
There was a problem hiding this comment.
But when does .blocks-gallery-grid.blocks-gallery-grid occur? This means an element will have the class twice?
There was a problem hiding this comment.
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' : '' }` }> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
There's an e2e test failure with the demo content. |
b2d23eb to
0641431
Compare
This reverts commit f4f8ba9.
79555cb to
2d43726
Compare
talldan
left a comment
There was a problem hiding this comment.
Looks good, just that question about galleryCaptionFocused that it'd be good to understand more about.
| } | ||
| if ( ! this.props.isSelected && prevProps.isSelected && this.state.galleryCaptionFocused ) { | ||
| this.setState( { | ||
| galleryCaptionFocused: false, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, well spotted. Removed the state altogether.
| value={ caption } | ||
| unstableOnFocus={ this.onFocusGalleryCaption } | ||
| onChange={ ( value ) => setAttributes( { caption: value } ) } | ||
| isSelected={ this.state.galleryCaptionFocused } |
There was a problem hiding this comment.
Related to the previous comment, I wonder how this could ever be true.
talldan
left a comment
There was a problem hiding this comment.
I know this one wasn't straightforward, great work here @tellthemachines!
|
It looks like block alignment might not have been handled by this PR: #17584 |
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
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: