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

Update App store screenshots #13211

Merged
merged 87 commits into from
Nov 20, 2020
Merged

Update App store screenshots #13211

merged 87 commits into from
Nov 20, 2020

Conversation

JavonDavis
Copy link
Contributor

This PR makes updates to the mocks and the screenshot tests. Apologies for this being a bit large the changes were a bit incremental and built upon the changes in #13082, #13031 and #13123.

Changes in this PR:

  • Updates the mocks used for the screenshot tests
  • Adds new util to switch between dark and light mode
  • Updates the Screenshot to navigate to the new screens
  • Updates the fastlane configuration file for the screenshots
  • Updates the phone device used for the test to API 30, this was not done for tablet due to a bug that causes auto rotate to not happen

To test:

  • Run the screenshot test
  • bundle exec fastlane screenshots locale:"<locale>" device:"<phone or tenInch>" should generate the screenshots
  • bundle exec fastlane android create_promo_screenshots locale:"<locale>"

Javon Davis and others added 30 commits September 21, 2020 10:53
…/WordPress-Android into update/wp-screenshots
…/WordPress-Android into update/screenshot-ui-tests-with-new-designs
…id into update/screenshot-ui-tests-with-new-designs
…com:wordpress-mobile/WordPress-Android into update/mocks-tmp
Comment on lines 76 to 80
"text": "playstoreres/metadata/%s/play_store_screenshot_2.txt",
"size": [750,350],
"position": [157,0 ],
"alignment": "center",
"stylesheet": "playstoreres/assets/intro-text.css"
Copy link
Member

Choose a reason for hiding this comment

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

Rather than repeat a text attachment like this for all the screenshots (since most of the values are the same each time), you can set the text styles in the global stylesheet playstoreres/assets/style.css (defined in line 5) and set the text_size and text_offset for the device (e.g. line 22 for the Pixel 3) rather than set a size and position for each screenshot.

Then, you can set the text value for each screenshot like this:

		// Create a site or start a blog
		{
			"device": "Pixel 3",
			"screenshot": "images/phoneScreenshots/1-create-a-site-or-start-a-blog.png",
			"background": "playstoreres/assets/attachments/phone-backgrounds/Circles-2.png",
			"text": "playstoreres/metadata/%s/play_store_screenshot_2.txt"
		},

The exception to that is the first screenshot, since the text has to be added as an attachment there to overlay the other file attachments.

@AliSoftware AliSoftware self-requested a review November 12, 2020 21:36
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

🎉

// Get a screenshot of the drafts feature
screenshotPostWithName("Ideas", "6-capture-ideas-on-the-go", false);
// Get a screenshot of the editor with the block library expanded
String name = "1-create-a-site-or-start-a-blog";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know this was already implemented that way before your changes, but wondering why we create an intermediate String name variable here – while it's only used once, just 2 lines below – instead of using the string directly on line 105 🤷‍♂️

Not a big deal at all, you can ignore this nit and keep as is, but just felt curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'll leave as is, at the very least it keeps the line shorter 🤷🏽‍♂️

swipeDownOnView(R.id.scroll_view);
moveToStats();

idleFor(2000);
Copy link
Contributor

Choose a reason for hiding this comment

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

moveToStats() already include a idleFor(8000) at the end of its code (and moveToStats is only called from here), so you might want to combine the two idleFor(8000) and idleFor(2000) in a single idleFor(10000) – or reduce that duration if the long pause was previously used more for debugging – and only keep one.

(Not sure which of the two is the best to keep (end of moveToStats or here 🤷 ), I'll let you decide)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stats can take a while to load so I'll leave the time as is in the moveToStats but I've removed this second one, it's not necessary anymore!

@@ -0,0 +1,6 @@
*{
font-family: 'Noto Serif';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR (as the potential risk was true before those changes too I think), but commenting here to keep it in mind:

Is there a risk that this font could not installed on the machine the user is using to generate the screenshots (or, on CI)? If that's the case, there' a risk the HTML rendering uses a substitution or default system font instead I think, which could easily then be overlooked, especially if the person reviewing the screenshots doesn't know what the font is supposed to look like…

Maybe we could add a check about that in our scripts generating the final promo screenshots 🤔 worth to keep in mind at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll open a separate issue to track this, thanks for bringing it up!

@JavonDavis JavonDavis merged commit f83e997 into develop Nov 20, 2020
@JavonDavis JavonDavis deleted the update/wp-screenshots branch April 17, 2021 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants