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

PaulREnglish · 2024-10-08T11:40:05Z

Description

Assigns the Image Comparisons containerHeight and containerWidth values a default of null. This is to avoid an issue where the block won't scale to the new width if the settings.layout.contentSize setting in theme.json is changed and the user has not set a width yet.

Fixes an issue where the alignment options in the block toolbar have no effect due to it being overridden by the containerHeight and containerWidth attribute styles that are passed into the ResizableBox component. Instead, we now set the containerWidth and containerHeight to auto if the alignment attribute has been set.

Change Log

Fixes an issue where containerHeight and containerWidth values not specifically set the by user would not scale when the settings.layout.contentSize setting in theme.json would change.
Fixes an issue where the alignment options in the block toolbar have no effect.

Steps to test

Issue #28

Add an Image Comparison block
Add both images
Save
Increase the settings.layout.contentSize value in theme.json
Refresh page
See that the Image Comparison block is now displaying correctly in the editor and on the frontend

Issue #29

Add an Image Comparison block
Add both images
Apply a settings.layout alignment value
The width and height settings within the block sidebar should disappear.
The alignment of the block should be reflected correctly both within the editor and on the frontend.

Screenshots/Videos

http://bigbite.im/v/KfN5NC (#28)

http://bigbite.im/v/3piqUx (#29)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

Fixes an issue where editing settings.layout.contentSize in theme.json would not change the width of the block.

This was needed to fix 'Arrow function has a complexity of 12. Maximum allowed is 10' JS lint error

This reverts commit 8c657d8.

This was needed to fix 'Arrow function has a complexity of 12. Maximum allowed is 10' JS lint error

…ment-override

chrishbite

Hey @PaulREnglish thanks for the work on this, I've left a couple of changes below on this please.

I've also consolidated the dependent PRs description/testing steps into this one and closed 38 as these will be easier to review and test them together with code overlapping and being dependent on each other.

1) These changes will generate the below PHP warnings from image-comparison/render.php with default usage.

Warning: Undefined array key "align"
Warning: Undefined array key "containerHeight"
Warning: Undefined array key "align"
Warning: Undefined array key "containerWidth"

2) I should of been clearer in the issue here, so apologies for that.

The ResizableBox and WIDTH / HEIGHT settings panel should still be available when using Align left, Align center and Align right alignment options otherwise you cannot correctly control the block when these are set.

The reference to the settings.layout values in the issue was meant to be specifically for the values you can supply to this property in theme.json, so apologies for not being clear on that.

Again thanks for the work on this, if you need any assistance or would like to go over any of the points raised please feel free to reach out.

chrishbite · 2024-10-31T10:25:08Z

src/blocks/image-comparison/components/Edit.js

+  /**
+   * 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();


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?

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.

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 👍

Will do thanks!

PaulREnglish added 10 commits October 7, 2024 14:55

fix: assign height and width a default value of null

3daf7b4

Fixes an issue where editing settings.layout.contentSize in theme.json would not change the width of the block.

fix(image-comparison): move block props into own function

8c657d8

This was needed to fix 'Arrow function has a complexity of 12. Maximum allowed is 10' JS lint error

Revert "fix(image-comparison): move block props into own function"

d7a2b22

This reverts commit 8c657d8.

fix(image comparison): create function for getting container size

97c4c27

This was needed to fix 'Arrow function has a complexity of 12. Maximum allowed is 10' JS lint error

docs: fix invalid getContainerSize typehinting in docblock

6edfadc

fix(image-comparison): allows alignment settings to affect the block

1b5d327

docs: correct return param type in getContainerSize docblock

e304e01

Merge branch 'fix/width-height-default-null' into fix/no-layout-align…

360a294

…ment-override

fix: fix alignment functionality on front end

812ffc0

fix: hide width/height settings when alignment is set

30fe54b

PaulREnglish marked this pull request as ready for review October 8, 2024 13:40

PaulREnglish requested review from chrishbite, jaymcp and a team as code owners October 8, 2024 13:40

PaulREnglish added 2 commits October 8, 2024 14:47

docs: improve getContainerSize docblock description

71f341a

Merge branch 'fix/width-height-default-null' into fix/no-layout-align…

a95da67

…ment-override

PaulREnglish mentioned this pull request Oct 8, 2024

Issues with responsiveness when saving with default width/height configuration #31

Open

PaulREnglish changed the title ~~Fixes settings.layout alignment values are being overridden~~ [29] Fixes settings.layout alignment values are being overridden Oct 8, 2024

chrishbite mentioned this pull request Oct 31, 2024

[28] Fixes container width and height values causing layout issues when changing theme or updating settings.layout.contentSize value in theme.json #38

Closed

6 tasks

chrishbite changed the title ~~[29] Fixes settings.layout alignment values are being overridden~~ [28][29][31] Width and Height block settings Oct 31, 2024

This was linked to issues Oct 31, 2024

settings.layout alignment values are being overridden and have no effect when set on the block #29

Open

Issues with responsiveness when saving with default width/height configuration #31

Open

Width and Height block settings tracking issue #32

Open

chrishbite requested changes Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PaulREnglish commented Oct 8, 2024 •

edited by chrishbite

Loading

chrishbite left a comment

chrishbite Oct 31, 2024

PaulREnglish Oct 31, 2024

chrishbite Oct 31, 2024 •

edited

Loading

PaulREnglish Oct 31, 2024

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

Are you sure you want to change the base?

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

Conversation

PaulREnglish commented Oct 8, 2024 • edited by chrishbite Loading

Description

Change Log

Steps to test

Issue #28

Issue #29

Screenshots/Videos

Checklist:

chrishbite left a comment

Choose a reason for hiding this comment

chrishbite Oct 31, 2024

Choose a reason for hiding this comment

PaulREnglish Oct 31, 2024

Choose a reason for hiding this comment

chrishbite Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

PaulREnglish Oct 31, 2024

Choose a reason for hiding this comment

PaulREnglish commented Oct 8, 2024 •

edited by chrishbite

Loading

chrishbite Oct 31, 2024 •

edited

Loading