[Site Design Revamp] Re-enable large recommended theme screenshots #16848
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tations." This reverts commit 78fee08.
We use the `ThumbDimensionProvider` for displaying as well as fetching preview screenshots with the correct dimensions. The values are automatically scaled based on screen density on the client side, but for legacy reasons, the API endpoint *also* scales the dimensions, resulting in the scale being applied twice. To avoid this, we divide by the scale factor when computing the dimension parameters to fetch the designs. This can be revisited in the future if we change or version the REST endpoint to remove the middleware scaling behavior.
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
Contributor
You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16848-8cc4806.apk), or scanning this QR code:
|
Contributor
You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16848-8cc4806.apk), or scanning this QR code:
|
antonis
approved these changes
Aug 2, 2022
Contributor
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Great work @mkevins 👍
I've tested the implementation on a Pixel 5 (Android 12) and on a Pixel C tablet emulator. Everything worked as expected for me. The code changes also look good and the explanatory comments should be helpful in the future 🎉
Thank you for fixing this 🙇
Contributor
Author
|
Thank you for reviewing and testing! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #16847
Description
This PR re-enables the larger screenshots disabled as a work-around in #16747. Formerly, the
scaleparameter was not being passed (or defaulted to 1).Since this parameter is needed to achieve properly sized screenshots for higher screen densities, this PR passes this value based on the device's screen density. Since the dimensions are already scaled by this value (by default when retrieving the dimension resource in Android), this PR manually. But, since the backend also uses the scale parameter to calculate the correct dimensions, to avoid doing this transformation twice, we divide by this value to calculate the correct dimensions for the request parameters. This is needed so that we can maintain API compatibility with older client versions.
Without this workaround, the scale will be applied twice, resulting in larger images than necessary (and longer download times, more data usage, and decreased performance).
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 back-end changes have been deployed).The patch should be applied to the sandbox fromD83271-codePage 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
Potential unintended areas of impact
Page picker
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual tests
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:
RELEASE-NOTES.txtif necessary.