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.

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

Tiled Gallery Block: Aspect Ratio for Square Layout #4017

Merged
merged 10 commits into from
Nov 2, 2021

Conversation

SiobhyB
Copy link
Contributor

@SiobhyB SiobhyB commented Sep 23, 2021

Jetpack: Automattic/jetpack#21166

Description

Fixes: #4010

This PR is one part of a wider effort to port the Jetpack Tiled Gallery block to Mobile Gutenberg. It will be merged into a feature branch, add/tiled-gallery-block.

For this PR chain, the overarching purpose is to define the aspect ratio of images in the editor, so that they appear as squares.

Testing

Please refer to the Jetpack PR as the central PR with the most up-to-date testing instructions.


PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 28, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@SiobhyB SiobhyB changed the title Tiled Gallery Block: Aspect Ratio Tiled Gallery Block: Aspect Ratio for Square Layout Sep 28, 2021
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

I reviewed the Jetpack PR here but will hold off reviewing the Gutenberg PR to give others a chance to validate the approach (because it involves adding changes to the core blocks).
Feel free to re-request a review here when the Jetpack and Gutenberg PRs are ready for review again.

While it would be nice to add integration tests to our Gutenberg Mobile PRs such as this, I don't think verifying the aspect ratio would be a suitable thing to test. It doesn't affect the HTML representation of the block – it's just a visual in-editor change – so it seems like a difficult piece of code to test, at least with integration tests.

@SiobhyB SiobhyB requested a review from guarani October 28, 2021 16:01
@guarani
Copy link
Contributor

guarani commented Oct 28, 2021

The following sibling PRs include complementary changes:

Should the above be deleted or are there sibling PRs here?

On a different topic, would it make sense to use the Fixes keyword in the PR description to close #4010?

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

@SiobhyB
Copy link
Contributor Author

SiobhyB commented Oct 29, 2021

Should the above be deleted or are there sibling PRs here?

You're right, I overlooked this line when editing the PR description, updated now. Thanks for pointing that out!

On a different topic, would it make sense to use the Fixes keyword in the PR description to close #4010?

Sounds good to me, added that now.

@guarani guarani self-requested a review October 29, 2021 14:22
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Approved via Automattic/jetpack#21166 (review). I think the failures here are related to the single Tiled Gallery integration test. This test will be disabled in #4143 so it might be worth checking the state of that PR and seeing if it resolves the failures here.

cc @illusaen

@SiobhyB
Copy link
Contributor Author

SiobhyB commented Oct 29, 2021

I think the error may be related to the fact I recently updated the feature branch from develop as well as the changes to the Jetpack tests in #4008. It looks like this file on our feature branch should be deleted: https://github.com/wordpress-mobile/gutenberg-mobile/blob/add/tiled-gallery-block/src/jetpack/test/tiled-gallery.js

That said, I expect that there are probably other changes that will also be needed, maybe warranting their own PR. I'll be happy to do some more digging into that on Monday unless others have insights before then.

@SiobhyB SiobhyB merged commit 392c454 into add/tiled-gallery-block Nov 2, 2021
@SiobhyB SiobhyB deleted the add/squared-aspect-ratio branch November 2, 2021 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks InnerBlocks Jetpack Bug or feature related to Jetpack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants