Design picker: Display a screenshot instead of mshots when externally managed #80573

WBerredo · 2023-08-14T17:14:05Z

Resolves https://github.com/Automattic/dotcom-forge/issues/3447
Related to D118900-code

Proposed Changes

Check if the theme for a given design is externally managed and display a screenshot instead of mshots
Remove the filter out for Marketplace producs
Query the Product List to receive Marketplace themes prices

Testing Instructions

Apply the diff D118900-code to your sandbox and direct requests to the sandbox
Go to the route /setup/update-design/designSetup?siteSlug=:site
The Marketplace Themes (ex: Yuna, Olsen) should have the partner tag and properly show the screenshot
Verify the price is available when hovering the partners tag

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

github-actions · 2023-08-14T17:14:20Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-78169

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-78169&env=jetpack

matticbot · 2023-08-14T18:28:23Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~427 bytes added 📈 [gzipped])

name                     parsed_size           gzip_size
update-design-flow            +417 B  (+0.0%)     +141 B  (+0.0%)
site-setup-flow               +417 B  (+0.0%)     +141 B  (+0.0%)
import-flow                   +417 B  (+0.0%)     +141 B  (+0.0%)
free-flow                     +417 B  (+0.0%)     +141 B  (+0.0%)
themes                        +263 B  (+0.0%)      +97 B  (+0.0%)
marketplace                   +263 B  (+0.0%)     +108 B  (+0.0%)
tailored-ecommerce-flow        +86 B  (+0.0%)      +42 B  (+0.0%)
link-in-bio-tld-flow           +86 B  (+0.0%)      +39 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~97 bytes added 📈 [gzipped])

name                                     parsed_size           gzip_size
async-load-signup-steps-theme-selection       +263 B  (+0.2%)      +97 B  (+0.2%)
async-load-design-blocks                      +263 B  (+0.0%)      +97 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

gcsecsey

Changes LGTM and work great:

WBerredo · 2023-08-15T16:44:55Z

On hold due to:

https://github.com/Automattic/dotcom-forge/issues/3447#issuecomment-1679276195

WBerredo · 2023-08-19T14:31:18Z

@gcsecsey can you give a new round of review with the updated test instructions?

WBerredo · 2023-08-19T14:33:00Z

This PR is not blocked anymore because it was decoupled from #80446 and can be shipped separately as soon as its reviewed.

gcsecsey

Changes LGTM, test well and work as described.

Hovering/touching the partner tag works great on both mobile and desktop:

Viewport	Screenshot
mobile
desktop

gcsecsey · 2023-08-21T10:53:07Z

packages/design-picker/src/components/unified-design-picker.tsx

@@ -36,6 +37,20 @@ const DesignPreviewImage: React.FC< DesignPreviewImageProps > = ( {
 } ) => {
 	const isMobile = useViewportMatch( 'small', '<' );

+	if ( design.is_externally_managed && design.screenshot ) {
+		const fit = '479,360';


Nit: Where do these numbers come from? Will these need to be updated in the future?

Currently the image fits nicely for all breakpoints:

Breakpoint Page

<320px

<768px

<1024px

Good point, I brought it from here where we also display the screenshot: https://github.com/Automattic/wp-calypso/blob/trunk/client/components/theme/index.jsx#L176

I didn't invest much in figuring out a way to share this value because it comes from client/components, and this is a separate package so it must have all the info by itself or be passed by param. In this case, I decided just to replicate the code.

Thanks for the context. It's good to know that the thumbnails are also a fixed aspect ratio. I wondered what was the original motivation behind moving the design picker to a separate package as it seems to cause some friction when we make updates. I've only found this point about the i18n wrapper: #51226 (comment)

I wondered what was the original motivation behind moving the design picker to a separate package

I already asked me the same thing, but I didn't find an answer yet.

mmtr

Remove the following code from the line 320: tier: '-marketplace',

I think it's safe to include that as part of this PR. That part of the logic does not control the themes that are visible in the grid, but the information that it's needed to correctly render the badges and tooltips.

That way, I think we can merge PRs/diffs in this order:

Design picker: Display a screenshot instead of mshots when externally managed #80573 (this PR, plus the removal of tier: '-marketplace').
1.1. Nothing will happen: Partner themes won't be visible since the starter-designs endpoint doesn't return then yet
1.2. Because of the above, we can abandon Design Picker: Query all themes (including Marketplace themes) #80446
Design Picker: Allow Marketplace plugins to be displayed as selected Theme #80837
More additional PRs
D118900-code
4.1. This is where Partner themes will become visible.

WBerredo · 2023-08-21T13:06:37Z

@mmtr Thanks for the suggestion, and it's a possibility I considered but decided against it, in the end, it would be nice to have other input.

Pro

Its more straightforward
It will help us in future reviews by removing the necessity of changing this line

Con

This will query unnecessary data (Marketplace themes) for now, even if we are not displaying it.

I'm good with both options, but I would like to put here my line of thought.

mmtr

This will query unnecessary data (Marketplace themes) for now, even if we are not displaying it.

That's fair! Note that we can also add a new feature flag. That way we don't fetch partner themes data in production by default, but can be enabled in development, so we don't need to remove that line.

WBerredo · 2023-08-21T14:19:44Z

Note that we can also add a new feature flag.

Aha! Good point, I'll add it.

WBerredo · 2023-08-21T19:36:47Z

The feature flag logic for querying themes is added to this PR after the merge of #80446 and the rebase of this PR.

WBerredo self-assigned this Aug 14, 2023

gcsecsey approved these changes Aug 15, 2023

View reviewed changes

WBerredo added the [Status] Blocked / Hold label Aug 15, 2023

WBerredo mentioned this pull request Aug 15, 2023

Onboarding Theme: Fix paid plugin displaying on the theme page #80571

Closed

WBerredo added the DO NOT MERGE label Aug 16, 2023

WBerredo force-pushed the update/onboard-themes-img branch from 667d818 to fccc200 Compare August 19, 2023 14:05

WBerredo requested a review from gcsecsey August 19, 2023 14:29

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 19, 2023

WBerredo removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Blocked / Hold DO NOT MERGE labels Aug 19, 2023

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 19, 2023

WBerredo requested a review from a team August 19, 2023 19:23

gcsecsey approved these changes Aug 21, 2023

View reviewed changes

mmtr reviewed Aug 21, 2023

View reviewed changes

WBerredo requested a review from mmtr August 21, 2023 13:06

mmtr approved these changes Aug 21, 2023

View reviewed changes

WBerredo mentioned this pull request Aug 21, 2023

Design Picker: Query all themes (including Marketplace themes) #80446

Merged

7 tasks

WBerredo added 3 commits August 21, 2023 16:25

Display a screenshot instead of mshots when externally managed

8b1adca

Fix unit tests

7250180

Make is_externally_managed optional

780914d

WBerredo force-pushed the update/onboard-themes-img branch from fccc200 to 780914d Compare August 21, 2023 19:25

WBerredo merged commit 3e932d4 into trunk Aug 22, 2023

WBerredo deleted the update/onboard-themes-img branch August 22, 2023 14:35

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Design picker: Display a screenshot instead of mshots when externally managed #80573

Design picker: Display a screenshot instead of mshots when externally managed #80573

WBerredo commented Aug 14, 2023 •

edited

Loading

github-actions bot commented Aug 14, 2023 •

edited by matticbot

Loading

matticbot commented Aug 14, 2023 •

edited

Loading

gcsecsey left a comment

WBerredo commented Aug 15, 2023

WBerredo commented Aug 19, 2023

WBerredo commented Aug 19, 2023

gcsecsey left a comment

gcsecsey Aug 21, 2023

WBerredo Aug 21, 2023 •

edited

Loading

gcsecsey Aug 21, 2023

WBerredo Aug 21, 2023

mmtr left a comment •

edited

Loading

WBerredo commented Aug 21, 2023

mmtr left a comment

WBerredo commented Aug 21, 2023

WBerredo commented Aug 21, 2023

Design picker: Display a screenshot instead of mshots when externally managed #80573

Design picker: Display a screenshot instead of mshots when externally managed #80573

Conversation

WBerredo commented Aug 14, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Aug 14, 2023 • edited by matticbot Loading

matticbot commented Aug 14, 2023 • edited Loading

gcsecsey left a comment

Choose a reason for hiding this comment

WBerredo commented Aug 15, 2023

WBerredo commented Aug 19, 2023

WBerredo commented Aug 19, 2023

gcsecsey left a comment

Choose a reason for hiding this comment

gcsecsey Aug 21, 2023

Choose a reason for hiding this comment

WBerredo Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

gcsecsey Aug 21, 2023

Choose a reason for hiding this comment

WBerredo Aug 21, 2023

Choose a reason for hiding this comment

mmtr left a comment • edited Loading

Choose a reason for hiding this comment

WBerredo commented Aug 21, 2023

mmtr left a comment

Choose a reason for hiding this comment

WBerredo commented Aug 21, 2023

WBerredo commented Aug 21, 2023

WBerredo commented Aug 14, 2023 •

edited

Loading

github-actions bot commented Aug 14, 2023 •

edited by matticbot

Loading

matticbot commented Aug 14, 2023 •

edited

Loading

WBerredo Aug 21, 2023 •

edited

Loading

mmtr left a comment •

edited

Loading