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

Jump to bottom

[28][29][31] Width and Height block settings #39

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[28][29][31] Width and Height block settings #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3daf7b4
fix: assign height and width a default value of null
PaulREnglish Oct 7, 2024
8c657d8
fix(image-comparison): move block props into own function
PaulREnglish Oct 7, 2024
d7a2b22
Revert "fix(image-comparison): move block props into own function"
PaulREnglish Oct 7, 2024
97c4c27
fix(image comparison): create function for getting container size
PaulREnglish Oct 8, 2024
6edfadc
docs: fix invalid getContainerSize typehinting in docblock
PaulREnglish Oct 8, 2024
1b5d327
fix(image-comparison): allows alignment settings to affect the block
PaulREnglish Oct 8, 2024
e304e01
docs: correct return param type in getContainerSize docblock
PaulREnglish Oct 8, 2024
360a294
Merge branch 'fix/width-height-default-null' into fix/no-layout-align…
PaulREnglish Oct 8, 2024
812ffc0
fix: fix alignment functionality on front end
PaulREnglish Oct 8, 2024
30fe54b
fix: hide width/height settings when alignment is set
PaulREnglish Oct 8, 2024
71f341a
docs: improve getContainerSize docblock description
PaulREnglish Oct 8, 2024
a95da67
Merge branch 'fix/width-height-default-null' into fix/no-layout-align…
PaulREnglish Oct 8, 2024
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: 2 additions & 2 deletions inc/blocks/image-comparison/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@
},
"containerHeight": {
"type": "string",
"default": "500px"
"default": null
},
"containerWidth": {
"type": "string",
"default": "500px"
"default": null
}
},
"supports": {
Expand Down
4 changes: 2 additions & 2 deletions inc/blocks/image-comparison/render.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@
$extra_styles[] = sprintf( '--bigbite-image-comparison-divider-box-height: %s;', $attributes['dividerBoxHeight'] );
$extra_styles[] = sprintf( '--bigbite-image-comparison-divider-box-border-radius: %s;', $attributes['dividerBoxBorderRadius']['top'] );
$extra_styles[] = sprintf( '--bigbite-image-comparison-divider-icon-gap: %s;', $attributes['dividerIconGap'] );
$extra_styles[] = sprintf( '--bigbite-image-comparison-container-height: %s;', $attributes['containerHeight'] );
$extra_styles[] = sprintf( '--bigbite-image-comparison-container-width: %s;', $attributes['containerWidth'] );
$extra_styles[] = sprintf( '--bigbite-image-comparison-container-height: %s;', $attributes['align'] ? 'auto' : $attributes['containerHeight'] );
$extra_styles[] = sprintf( '--bigbite-image-comparison-container-width: %s;', $attributes['align'] ? 'auto' : $attributes['containerWidth'] );

foreach ( $colours as $colour ) {
if ( ( empty( $attributes[ $colour[0] ] ) && empty( $attributes[ $colour[1] ] ) ) || ! $colour[3] ) {
Expand Down
71 changes: 50 additions & 21 deletions src/blocks/image-comparison/components/Edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { ResizableBox } from '@wordpress/components';
import { useInnerBlocksProps, useBlockProps, useSettings } from '@wordpress/block-editor';
import { useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -46,10 +45,46 @@ const Edit = ({ attributes, setAttributes, clientId }) => {
customCaptionTextColour,
captionBackgroundColour,
customCaptionBackgroundColour,
containerHeight,
containerWidth,
} = attributes;

/**
* Get the container's size. If this has not been set by
* the user then we overwrite the default size of the block
* with the theme's defined contentSize, if it exists.
*
* @returns {
* width: string
* height: string
* } The size containing the height and width of the block's container.
*/
const getContainerSize = () => {
if (attributes.align) {
return {
containerWidth: 'auto',
containerHeight: 'auto',
};
}

let containerHeight = '500px';
if (attributes.containerHeight) {
containerHeight = attributes.containerHeight;
}

let containerWidth = '500px';
if (attributes.containerWidth) {
containerWidth = attributes.containerWidth;
} else if (contentWidth) {
containerWidth = contentWidth;
}

return {
containerHeight,
containerWidth,
};
};

const { containerHeight, containerWidth } = getContainerSize();
Comment on lines +50 to +86
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this function will be needed? If the containerHeight and containerWidth attributes aren't set then the block just needs to respond to it's parent element, the nullish coalescing operator (??) might hopefully be enough to handle this?

I'm not sure where the 500px default width and height values have come from as currently this will lead to a visual discrepancy between editor and frontend, steps to reproduce this;

  • Add Image Comparison block
  • Add 2 images
  • Save

The block in editor will end up with the height of 500px, if you then visit the frontend you'll see that the block is displaying as expected (height scaling with the width), are the 500px values required?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review! I added the 500px values to make it backwards compatible as that was the default value used in block.json before I changed it to null. (See here)

I'll have a look into the visual discrepancy and see what I can do about it.

Copy link
Collaborator

@chrishbite chrishbite Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @PaulREnglish thanks for the explanation, I think we may be able to avoid needing to do that and should try to avoid setting any width or height pixel values until the user specifically sets them and let the original image size then respond to it's container by default.

I'm not sure of the reason they were set to default to 500px originally as well, I may be overlooking something here though but please let me know if I can assist with anything on this 👍

Copy link
Author

Choose a reason for hiding this comment

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

Will do thanks!


const innerBlockSettings = {
template: [['bigbite/image-comparison-item'], ['bigbite/image-comparison-item']],
templateLock: 'inserter',
Expand All @@ -61,31 +96,25 @@ const Edit = ({ attributes, setAttributes, clientId }) => {
*/
let shouldDisplayResize = false;

/**
* Overwrite the default size of the block with the theme's
* defined contentSize, if it exists. This should only be
* applied if no images have been added to the block.
*/
useEffect(() => {
if (!shouldDisplayResize) {
setAttributes({ containerWidth: contentWidth });
}
}, [contentWidth]);

/**
* Retrieve the inner blocks
*/
const [{ innerBlocks }] = wp.data.select('core/block-editor').getBlocksByClientId(clientId);

/**
* Determine whether to allow the resize handles to be
* displayed based on if an image is assigned or not
* Don't display resize handles if the user has chosen one of the alignment settings.
*/
innerBlocks.forEach((block) => {
if (block?.attributes?.id) {
shouldDisplayResize = true;
}
});
if (!attributes.align) {
/**
* Determine whether to allow the resize handles to be
* displayed based on if an image is assigned or not
*/
innerBlocks.forEach((block) => {
if (block?.attributes?.id) {
shouldDisplayResize = true;
}
});
}

/**
* Only ever display the right, bottom, and bottomRight handles
Expand Down