Skip to content

[Site Design Revamp] Re-enable large recommended theme screenshots #18972

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.

Sign up for GitHub

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

Jump to bottom
Merged
mkevins merged 5 commits into from
Aug 1, 2022
Merged

[Site Design Revamp] Re-enable large recommended theme screenshots #18972

mkevins merged 5 commits into from
Aug 1, 2022

Conversation

@mkevins
Copy link
Contributor

@mkevins mkevins commented Jul 1, 2022 •
edited
Loading

Fixes #18971

Description

This PR re-enables the larger screenshots disabled as a work-around in #18879. This PR also fetches the screenshots for the theme picker using the dimensions from the "recommended" sizes (rather than "category" sizes). Other changes this PR relies on are on the API side (D83271-code).

Testing steps

Pre-conditions:

  • Device should be connected to a WPCOM sandbox
  • The patch should be applied to the sandbox from D83271-code
  1. Navigate to the site creation flow
  2. Choose an Intent (e.g. "Food")
  3. Expect the recommended for you screenshots to be larger
  4. Expect the screenshots to be loaded with proper display resolution (i.e. not pixelated)
    • It can be helpful to inspect traffic (e.g. in Charles) to ensure that properly sized screenshots are requested.
    • Inspecting traffic can also help confirm that the returned screenshots are the correct size (limited to 1200 width).

Page picker

The page picker should not be affected by these changes (though backend changes in the API may result in changes to the fetched screenshot URLs)

Regression Notes

  1. Potential unintended areas of impact
    Page picker

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual tests

  3. What automated tests I added (or what prevented me from doing so)
    Establishing the required testing apparatus for this would be out of scope for this task.

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.

mkevins added 3 commits June 30, 2022 14:48
@mkevins
Revert "Reduce recommended design thumbnail sizes due to backend limi…
aa90daf 
…tations."

This reverts commit 1eb6b22.
@mkevins
Scale the requested dimensions by the native screen density
7ef62ec
@mkevins
Remove scaling on the client (dimensions will be scaled on the backend)
604cd28 
For now, to keep compatibility with older versions of the client, we'll
keep the server-side behavior to scale dimensions in the middleware
layer intact. We can revisit later and shift this responsibility to the
client if we choose to change / version the endpoint.
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 1, 2022 •
edited
Loading

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 pr18972-e262c27 on your iPhone

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 1, 2022 •
edited
Loading

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 pr18972-e262c27 on your iPhone

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

@mkevins mkevins requested a review from twstokes July 1, 2022 07:52
twstokes
twstokes approved these changes Jul 3, 2022
View reviewed changes
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

👋 Good work @mkevins! Everything worked as described. 🎉

I have some thoughts on the web side (D83271-code) regarding payload sizes which I'll bring up outside of this PR. 🙇

@mkevins mkevins added this to the 20.5 milestone Aug 1, 2022
@mkevins mkevins marked this pull request as ready for review August 1, 2022 22:25
@mkevins mkevins enabled auto-merge August 1, 2022 22:45
@mkevins mkevins merged commit 1dd2957 into trunk Aug 1, 2022
@mkevins mkevins deleted the try/re-enable-large-recommended-theme-screenshots branch August 1, 2022 22:48
@mkevins
Copy link
Contributor Author

mkevins commented Aug 2, 2022

Thank you for reviewing and testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@twstokes twstokes twstokes approved these changes

Assignees

No one assigned

Labels

Site Creation [Type] Enhancement

Projects

None yet

Milestone

20.5

Development

Successfully merging this pull request may close these issues.

Re-enable large recommended theme previews

4 participants

@mkevins @wpmobilebot @twstokes