Conversation
There was a problem hiding this comment.
Nice job @JavonDavis ! Everything works great and I think this is good to merge (once the conflict is resolved)! 🎉🚢
I added a couple of comments, but they're not issues with the code as much as just me trying to learn more about the codebase. 😀
| await editorPage.addNewImageBlock(); | ||
| let imageBlock = await editorPage.getImageBlockAtPosition( 1 ); | ||
|
|
||
| if ( ! isAndroid() ) { |
There was a problem hiding this comment.
Just asking for my own learning, why do our Android tests not need any of the code in these (rather large) blocks?
There was a problem hiding this comment.
On the emulator, clicking add image doesn't add an image to the image block, so that's why when running the test on iOS (Add Image > WordPress media library) it actually adds an image but on Android it doesn't. 🤔 I could probably move this code into a function to make it a little less obvious, WDYT?
There was a problem hiding this comment.
Thanks for explaining! That makes sense.
I do think it would be helpful to add a bit more context where we're doing these !isAndroid() checks. I don't have any strong preference between just adding a comment versus moving this code into a function, or even just creating a local "alias" along the lines of isCapableOfAddingImageToImageBlock() = () => ! isAndroid();, which would then be pretty descriptive if we used it in place of the !isAndroid() checks. Whatever you think would work best is fine with me.
There was a problem hiding this comment.
I just opted for more context in the comments
| //TODO: Improve the identifier for this element | ||
| return isAndroid() ? | ||
| await this.driver.elementByXPath( '//android.view.ViewGroup[@content-desc="Post title. Welcome to Gutenberg!"]/android.widget.EditText' ) : | ||
| await this.driver.elementByXPath( '//XCUIElementTypeOther[@name="Add title"]/XCUIElementTypeTextView' ); |
There was a problem hiding this comment.
Certainly not something that needs to be changed, but FWIW in code like this I generally find it nice to separate out the things that vary (the path) from the things that don't (method call).
const path = isAndroid() ?
'//android.view.ViewGroup[@content-desc="Post title. Welcome to Gutenberg!"]/android.widget.EditText' ) :
'//XCUIElementTypeOther[@name="Add title"]/XCUIElementTypeTextView' );
return await this.driver.elementByXPath(path);
There was a problem hiding this comment.
Ahh you're right I I usually go with that approach, thanks I can change this
…rg-mobile into add/tests-image-block
…rg-mobile into add/tests-image-block
Part of #745
To test: New tests are in
gutenberg-editor-image.test.jsIf you'd like to run those alone locally a quick way to do that is change the
describeblock of the other test files toxdescribeAlternatively run all the tests, they should all pass
Update release notes:
RELEASE-NOTES.txt.