[RNMobile] Fix social icons block alignment

[wpmobilebot]

Related PRs

• [RNMobile] Fix social icons block alignment WordPress/gutenberg#54100 by @derekblank

Description

This PR was generated by version-toolkit to downstream the changes for gutenberg submodule.

Additionally, this PR adds a UI test case to gutenberg-editor-sanity-test-1-visual.test to test the fix for #6133 and ensure a block outline is added for Social Link blocks added via the appender.

[peril-wordpress-mobile]

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

[derekblank]

@dcalhoun If you observe the updated image snapshots, you'll note that the top and bottom of the child Social Link block outline is cut off.

As the child Social Link block outline is flush with the parent Social Icon block and looks nice in the Editor itself, I believe this cut off is likely due to the way the screenshot element is being selected:

gutenberg-mobile/__device-tests__/gutenberg-editor-sanity-test-1-visual.test.js

Lines 371 to 376 in 7da5853

	const socialLinksBlockXpath = isAndroid()
	? '//android.widget.Button[@content-desc="Social Icons Block. Row 1"]'
	: '(//XCUIElementTypeOther[@name="Social Icons Block. Row 1"])[1]';
	const socialLinksBlock = await editorPage.driver.elementByXPath(
	socialLinksBlockXpath
	);

Let me know if you have any ideas of a way around this, or if you feel this might be an acceptable limitation screenshotting an element. We could always just capture the entire editorPage instead of only the Social Icon block, if that feels better from an image snapshot perspective. Another alternative would be to seek design feedback on if the child Social Link block outline margin should be smaller and "within" the boundaries of the parent Social Icon block. At the moment, this feels like an odd change to make based on the result of a UI snapshot, but seeking other perspectives.

Screen.Recording.2023-09-01.at.3.46.57.pm.mov

[derekblank]

I wasn't able to build the UI tests for iOS to capture the iOS image snapshots, which has been discussed on a separate issue at p1693536173206879-slack-C6UJ0KRKQ. I can resume this later once that issue is resolved, but if anyone reviewing can capture iOS image snapshots and wants to update, please feel free. 🙇

[dcalhoun]

If you observe the updated image snapshots, you'll note that the top and bottom of the child Social Link block outline is cut off.

My recommendation would be to retain usage of takeScreenshotByElement — it reduces the file size of snapshots and mitigates flaky tests — and leverage the padding argument it offers. This will ensure the border is fully captured in the snapshot. I went ahead and implemented this approach in f9bfdf2.

I left a few comments and commits for your review as well. Let me know what you think. 🙇🏻

[dcalhoun]

While I recognize the block outline is a highly visual element, I might argue that while its presence is important its style is not critical. I posit we could/should merely assert the functionality succeeds in the form of asserting the outline's presence in a traditional integration test. I.e. we could add a testID to the associated element and assert its presence rather than incurring the cost of an e2e visual snapshot test.

WDYT?

[derekblank]

I did consider the expense of the visual test, but I had not considered that the same behavior could be tested with an integration test asserting the presence of BlockOutline. As this current visual test addition asserts the behavior fixed in #6133, I suggest we keep the visual test and I can replace it with an integration test shortly afterward. Wydt?

[dcalhoun]

Sounds good. 👍🏻

[mokagio]

@dcalhoun @derekblank I recently merged #6153 and changed the GitHub Checks. In other PRs, eg #6152, I took it upon myself to merge trunk to unblock but here there's a conflicts here, so I'll just leave it till one of you will resolve them. Happy to help if needed.

[derekblank]

@mokagio I merged in trunk, and several of the CI tasks failed on CircleCI (example). The error seems to suggest that I needed to run npm install and commit the result, which updated the package.json in e4b87a1. Does it seem like these package.json changes be expected?

[derekblank]

My recommendation would be to retain usage of takeScreenshotByElement — it reduces the file size of snapshots and mitigates flaky tests — and leverage the padding argument it offers. This will ensure the border is fully captured in the snapshot. I went ahead and implemented this approach in f9bfdf2.

I hadn't considered the padding argument of takeScreenshotByElement. That makes sense -- thanks for pointing it out.

Thanks for making the updates!

[dcalhoun]

🚀 Thank you for writing tests.

@mokagio I merged in trunk, and several of the CI tasks failed on CircleCI (example). The error seems to suggest that I needed to run npm install and commit the result, which updated the package.json in e4b87a1. Does it seem like these package.json changes be expected?

This relates to #6159. It should be safe to merge the changes.