Skip to content

Jetpack Focus: Fix notifications screenshot for iPad - #19126

Merged
momo-ozawa merged 4 commits into
trunkfrom
fix/18960-ipad-screenshots
Aug 2, 2022
Merged

Jetpack Focus: Fix notifications screenshot for iPad#19126
momo-ozawa merged 4 commits into
trunkfrom
fix/18960-ipad-screenshots

Conversation

@momo-ozawa

@momo-ozawa momo-ozawa commented Jul 29, 2022

Copy link
Copy Markdown
Contributor

Part of #18960

Description

This PR fixes and issue where the notification details screen wasn't loading properly on iPad for the screenshot targets (i.e. WordPressScreenshotGeneration and JetpackScreenshotGeneration).

How to test

Test for both the WordPressScreenshotGeneration and JetpackScreenshotGeneration targets

  1. Run rake mocks
  2. Switch to the screenshot generation target
  3. Run testGenerateScreenshots() on an iPad simulator
  4. ✅ Verify: tests succeed and the screenshots match design specs
Before After
Simulator Screen Shot - iPad mini (6th generation) - 2022-07-22 at 16 34 47 Simulator Screen Shot - iPad mini (6th generation) - 2022-07-29 at 16 58 04 #2

Notes

FYI @shaunandrews @hassaanelgarem I removed the call to tap the reply button on the notification details screen. Previously, we were tapping the reply button which brought up the hardware keyboard onto the screen. For reference, this is what it used to look like. But the new notification details design fills up the screen, and bringing up the hardware keyboard would obscure a lot of the UI elements. Lmk what you think :)

Notif details screen Notif details screen w reply
Simulator Screen Shot - iPad mini (6th generation) - 2022-07-29 at 16 58 04 #2 Simulator Screen Shot - iPad mini (6th generation) - 2022-07-29 at 16 58 19

Regression Notes

  1. Potential unintended areas of impact
  • WordPressScreenshotGeneration target
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • Made sure the tests pass for WordPressScreenshotGeneration
  1. What automated tests I added (or what prevented me from doing so)
  • n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Momo Ozawa added 4 commits July 29, 2022 09:48
The comments details page wasn’t loading correctly because the site id didn’t match for the /me/sites endpoint and the /sites/<siteId>/comments/<commentId> endpoint.
@momo-ozawa momo-ozawa added Testing Unit and UI Tests and Tooling Jetpack App labels Jul 29, 2022
@momo-ozawa momo-ozawa added this to the 20.5 milestone Jul 29, 2022
@momo-ozawa momo-ozawa self-assigned this Jul 29, 2022
@wpmobilebot

Copy link
Copy Markdown
Contributor
You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19126-b3214c7 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot

Copy link
Copy Markdown
Contributor
You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19126-b3214c7 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@hassaanelgarem hassaanelgarem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works as described 🚀

@momo-ozawa
momo-ozawa merged commit 52660e5 into trunk Aug 2, 2022
@momo-ozawa
momo-ozawa deleted the fix/18960-ipad-screenshots branch August 2, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Jetpack App Testing Unit and UI Tests and Tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants