Skip to content

Conversation

thatisrich
Copy link
Contributor

@thatisrich thatisrich commented Jul 2, 2024

Description

Issue #9 - The image comparison block's dimensions are defined by the first image uploaded to the block. This change allows the dimensions of the block to be adjusted further to the initial size of the first image added.

Change Log

  • block.json new container height and width attributes have been added.
  • the settings panel been extended to include the height and width options.
  • within the editor, the image comparison block has the ability to alter the height and width of the block itself. Although the width can be adjusted outside of the block's container, the block itself does not overflow the container when rendered on the front end. This follows the WordPress core Image block's approach.
  • the front end render has been updated to reflect the height and width changes made in the editor

Steps to test

  • Check out the branch and run npm run build:prod to ensure up to date code
  • Add an image comparison block to a page
  • Notice, when no images are added, the resize handles are not present
  • Add an image to the block
  • Notice the handles become visible
  • After adding a second image, select the block and resize it using the handles on either the bottom, right, or bottom right of the block
  • Add a caption to the block
  • Select the block and, under the Settings panel, adjust the height and / or width of the block
  • Using the up / down arrow keys, the numbers can be incremented by 1. Holding shift and up / down will increase by 10
  • Save the changes and view the page on the front end
  • If the assigned width is larger than the container, notice the width of the block is locked at maximum width of that container

Screenshots/Videos

A video showing the resize container in action
A screenshot showing a width restricted block and caption

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

@jonnywatersbb
Copy link
Member

jonnywatersbb commented Jul 2, 2024

@thatisrich

Just having a look at this functionally and noticed that the height doesn't seem to be dynamically resizing in the same way as it might be in your demo - http://bigbite.im/v/laF0N8

Not sure if that's a version / browser issue?
Browser - Arc
Theme - Twenty Twenty-Four
Version - 6.5.5

@thatisrich
Copy link
Contributor Author

It looks like the CSS which contains the images isn't being picked up in the editor so the original image aspect ratio is being displayed.

I'm struggling to replicate it at the moment but it may be a browser / caching issue. I've mainly tested in Arc so will try in Chrome / Firefox 👍

@thatisrich
Copy link
Contributor Author

I'm unfortunately still struggling to replicate the image height bug that @jonnywatersbb experienced - testing carried out in Arc, Chrome, and Firefox.

Demo showing how the block works for me can be seen here

Steps taken with the aim to replicate the issue:

  • Updated version of WP from 6.3.2 to 6.5.5
  • Tried adding blocks via Appearance > Editor and also directly to a page via Pages
  • Adding a new block to the Blog Homepage
  • Adding a new block to a new Page (as shown in the demo below)
  • Using a variety of images (portrait and horizontal)
  • Checked out a previous branch, ran npm run build:prod, checked out the correct branch and re-ran npm run build:prod
  • Added a single block to a page
  • Added multiple blocks to a page

If anyone has any suggestions it would be greatly appreciated!

@jaymcp
Copy link
Member

jaymcp commented Jul 8, 2024

Interestingly, I'm seeing the same behaviour as @jonnywatersbb in Chrome running WP 6.6-RC2-58675 with both twentytwentythree and twentytwentyfour.

@thatisrich
Copy link
Contributor Author

Thanks for confirming @jaymcp, much appreciated.

Back to the drawing board to figure this one out!

@thatisrich
Copy link
Contributor Author

thatisrich commented Jul 9, 2024

In an attempt to recreate the block resize bug (where the images are displayed outside of the block's bounds), I created a new docker instance on a fresh URL and checked out the repo fresh into the new WP install.

I recorded another short demo showing the results (still unable to replicate, unfortunately).

Steps taken:

  1. run bbdocker create to set up the new environment
  2. check out the repo into the plugins folder of the new WP environment
  3. run npm install, npm run build:prod, and composer install within the plugin to ensure build is complete
  4. activate plugin in the CMS
  5. uploaded two large images to the media library
  6. navigate to a page, add the block, and select the two images
  7. tried resizing the block bounds to check if the images displayed outside of the container (images were contained and resized as expected)

I also tried following the same steps (of adding the block) in an incognito window and specifically clearing the browser cache and re-adding the block.

Details: WP 6.5, PHP 8.2, Arc v1.49.1 (51495)

@jonnywatersbb
Copy link
Member

Hey @jaymcp

Can you confirm if you have any other plugins running on the site when you are seeing the same bug as me?

I've just created a blank site and checked the PR out on the exact same setup as previous and didn't see the bug. Comparing the 2 environments, I have a feeling it's another plugin (probably Tabs) causing the issue.

@jaymcp
Copy link
Member

jaymcp commented Jul 9, 2024

@jonnywatersbb did a retest after deactivating all but required plugins... unfortunately i still see the same thing. i'll try with a fresh env on a stable wp release.

@jaymcp
Copy link
Member

jaymcp commented Jul 9, 2024

it might actually be an aspect ratio issue. if the container size is set in one dimension to be much larger than the image(s) can fill, i can replicate the issue.

this is in a fresh install running twentytwentyfour.

landscape images:

portrait images:

@thatisrich thatisrich linked an issue Jul 24, 2024 that may be closed by this pull request
@jonnywatersbb
Copy link
Member

jonnywatersbb commented Jul 24, 2024

@thatisrich I'm not seeing the issue I was on a blank WordPress install, so I'm wondering if all the other PR feedback from Jay has been addressed, if we look to approve and merge this along with #17 and do a full test of a release candidate?

Although I may need Jay or someone else to just approve the PR from a code POV

@thatisrich thatisrich requested a review from jaymcp July 24, 2024 14:39
@thatisrich
Copy link
Contributor Author

@jonnywatersbb The PR feedback has been taken care of so, if those changes are good to go, re-running tests after merge to a possible release branch sounds like a plan.

jaymcp
jaymcp previously approved these changes Aug 5, 2024
Copy link
Member

@jaymcp jaymcp left a comment

Choose a reason for hiding this comment

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

The code looks great (thanks for addressing the feedback, @thatisrich)!
Here's a new video demonstrating the issue we've been discussing:

image-comparison-issue-9.mp4

Approving the PR anyway, per the discussion in the ticket.

@ampersarnie
Copy link
Member

@thatisrich There's a couple of merge conflicts that we need resolving before we can merge this.

# Conflicts:
#	src/blocks/image-comparison/components/Edit.js
#	src/blocks/image-comparison/components/Settings.js
@thatisrich
Copy link
Contributor Author

@ampersarnie The merge conflict has been address and re-tested. A demo showing the block working after the conflict can be seen here. Thanks!

@thatisrich thatisrich requested a review from ampersarnie August 16, 2024 09:21
@jonnywatersbb jonnywatersbb requested a review from jaymcp August 19, 2024 09:00
@jaymcp jaymcp merged commit c5b444c into main Aug 19, 2024
@jaymcp jaymcp deleted the feature/9-allow-block-resize branch August 19, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow block to be resized

4 participants