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.

Sign up for GitHub

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

Gallery block refactor: remove the imageCount attribute #33677

Merged
merged 4 commits into from
Jul 26, 2021
Merged

Gallery block refactor: remove the imageCount attribute #33677

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8586af7
Remove the imageCount attribute and use CSS instead to set default co…
Jul 26, 2021
f1d7616
remove unneeded calc
Jul 26, 2021
427065f
Remove unnecessary nested calcs and fix lint error
aaronrobertshaw Jul 26, 2021
ba7ffcd
* Add optional chaining to innerBlockImages in edit wrapper in case i…
Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions packages/block-library/src/gallery/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,6 @@
"allowResize": {
"type": "boolean",
"default": false
},
"imageCount": {
"type": "number",
"default": 0
}
},
"providesContext": {
Expand Down
11 changes: 9 additions & 2 deletions packages/block-library/src/gallery/edit-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,14 @@ import EditWithoutInnerBlocks from './v1/edit';
* use of hooks lint errors if adding this logic to the top of the edit component.
*/
export default function GalleryEditWrapper( props ) {
const { attributes } = props;
const { attributes, clientId } = props;

const innerBlockImages = useSelect(
( select ) => {
return select( blockEditorStore ).getBlock( clientId )?.innerBlocks;
},
[ clientId ]
);

const __unstableGalleryWithImageBlocks = useSelect( ( select ) => {
const settings = select( blockEditorStore ).getSettings();
Expand All @@ -26,7 +33,7 @@ export default function GalleryEditWrapper( props ) {
// This logic is used to infer version information from content with higher
// precedence than the flag. New galleries (and existing empty galleries) will
// honor the flag.
const hasNewVersionContent = !! attributes?.imageCount;
const hasNewVersionContent = !! innerBlockImages.length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be safest to make it !! innerBlockImages?.length, just in case innerBlocks is ever undefined for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh, that makes sense, done.

const hasOldVersionContent =
0 < attributes?.ids?.length || 0 < attributes?.images?.length;
if (
Expand Down
24 changes: 8 additions & 16 deletions packages/block-library/src/gallery/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ function GalleryEdit( props ) {
} = props;

const {
imageCount,
columns = defaultColumnsNumber( imageCount ),
columns,
imageCrop,
linkTarget,
linkTo,
Expand Down Expand Up @@ -155,17 +154,6 @@ function GalleryEdit( props ) {
setAttributes( { shortCodeTransforms: undefined } );
}, [ shortCodeTransforms, shortCodeImages ] );

useEffect( () => {
if ( ! images ) {
setAttributes( { imageCount: undefined } );
return;
}

if ( images.length !== imageCount ) {
setAttributes( { imageCount: images.length } );
}
}, [ images ] );

const imageSizeOptions = useImageSizes(
imageData,
isSelected,
Expand All @@ -181,8 +169,8 @@ function GalleryEdit( props ) {
* it already existed in the gallery. If the image is in fact new, we need
* to apply the gallery's current settings to the image.
*
* @param {Object} existingBlock Existing Image block that still exists after gallery update.
* @param {Object} image Media object for the actual image.
* @param {Object} existingBlock Existing Image block that still exists after gallery update.
* @param {Object} image Media object for the actual image.
* @return {Object} Attributes to set on the new image block.
*/
function buildImageAttributes( existingBlock, image ) {
Expand Down Expand Up @@ -462,7 +450,11 @@ function GalleryEdit( props ) {
{ images.length > 1 && (
<RangeControl
label={ __( 'Columns' ) }
value={ columns }
value={
columns
? columns
: defaultColumnsNumber( images.length )
}
onChange={ setColumnsNumber }
min={ 1 }
max={ Math.min( MAX_COLUMNS, images.length ) }
Expand Down
13 changes: 1 addition & 12 deletions packages/block-library/src/gallery/gallery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ import {
import { __ } from '@wordpress/i18n';
import { createBlock } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import { defaultColumnsNumber } from './shared';

const allowedBlocks = [ 'core/image' ];

export const Gallery = ( props ) => {
Expand All @@ -37,13 +32,7 @@ export const Gallery = ( props ) => {
blockProps,
} = props;

const {
imageCount,
align,
columns = defaultColumnsNumber( imageCount ),
caption,
imageCrop,
} = attributes;
const { align, columns = 'default', caption, imageCrop } = attributes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of setting the default classname, but not sure about defaulting this variable to a string, since it's a numeric type usually, and I think it'd be good to keep the types consistent.

An alternative could be to change the classnames call below:

{
	[ `align${ align }` ]: align,
	[ `columns-${ columns }` ]: columns !== undefined,
	[ `columns-default` ]: columns === undefined,
	'is-cropped': imageCrop,
}

The same comment would also apply in the save function, which has the same code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, confusing the type there isn't great, switched to your suggested approach.


const { children, ...innerBlocksProps } = useInnerBlocksProps( blockProps, {
allowedBlocks,
Expand Down
1 change: 0 additions & 1 deletion packages/block-library/src/gallery/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export const settings = {
example: {
attributes: {
columns: 2,
imageCount: 2,
},
innerBlocks: [
{
Expand Down
8 changes: 1 addition & 7 deletions packages/block-library/src/gallery/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,14 @@ import { RichText, useBlockProps, InnerBlocks } from '@wordpress/block-editor';
/**
* Internal dependencies
*/
import { defaultColumnsNumber } from './shared';
import saveWithoutInnerBlocks from './v1/save';

export default function saveWithInnerBlocks( { attributes } ) {
if ( attributes?.ids?.length > 0 || attributes?.images?.length > 0 ) {
return saveWithoutInnerBlocks( { attributes } );
}

const {
imageCount,
caption,
columns = defaultColumnsNumber( imageCount ),
imageCrop,
} = attributes;
const { caption, columns = 'default', imageCrop } = attributes;

const className = `blocks-gallery-grid has-nested-images columns-${ columns } ${
imageCrop ? 'is-cropped' : ''
Expand Down
22 changes: 20 additions & 2 deletions packages/block-library/src/gallery/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,21 +158,39 @@
@for $i from 3 through 8 {
&.columns-#{ $i } figure.wp-block-image:not(#individual-image) {
margin-right: var(--gallery-block--gutter-size, #{$grid-unit-20});
width: calc(#{100% / $i} - calc(var(--gallery-block--gutter-size, #{$grid-unit-20}) * calc(#{$i - 1}) / #{$i}));
width: calc(#{100% / $i} - (var(--gallery-block--gutter-size, #{$grid-unit-20}) * #{$i - 1} / #{$i}));
}

// Prevent collapsing margin while sibling is being dragged.
&.columns-#{$i} figure.wp-block-image:not(#individual-image).is-dragging ~ figure.wp-block-image:not(#individual-image) {
margin-right: var(--gallery-block--gutter-size, #{$grid-unit-20});
}
}

// Unset the right margin on every rightmost gallery item to ensure center balance.
@for $column-count from 1 through 8 {
&.columns-#{$column-count} figure.wp-block-image:not(#individual-image):nth-of-type(#{ $column-count }n) {
margin-right: 0;
}
}
// If number of columns not explicitly set default to 3 columns if 3 or more images.
&.columns-default {
figure.wp-block-image:not(#individual-image) {
margin-right: var(--gallery-block--gutter-size, #{$grid-unit-20});
width: calc(33.33% - (var(--gallery-block--gutter-size, 16px) * 2 / 3));
}
figure.wp-block-image:not(#individual-image):nth-of-type(3n+3) {
margin-right: 0;
}
// If only 2 child images use 2 columns.
figure.wp-block-image:not(#individual-image):first-child:nth-last-child(2),
figure.wp-block-image:not(#individual-image):first-child:nth-last-child(2) ~ figure.wp-block-image:not(#individual-image) {
width: calc(50% - (var(--gallery-block--gutter-size, 16px) / 2));
}
// For a single image set to 100%.
figure.wp-block-image:not(#individual-image):first-child:nth-last-child(1) {
width: 100%;
}
}
}

// Apply max-width to floated items that have no intrinsic width.
Expand Down
1 change: 0 additions & 1 deletion packages/block-library/src/gallery/transforms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ const transforms = {
return createBlock(
'core/gallery',
{
imageCount: innerBlocks.length,
align,
sizeSlug,
},
Expand Down