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

Design Picker: Include premium themes #59293

Merged
merged 15 commits into from Jan 20, 2022

Conversation

arthur791004
Copy link
Contributor

@arthur791004 arthur791004 commented Dec 16, 2021

Changes proposed in this Pull Request

  • Check the premium themes are available for the user by planHasFeature utils and then determine the value of the tier should be all or free
  • Add PremiumBadge component in the Design Picker package and pass PremiumBadge as a prop of the Design Picker

Example of the premium badge and the hover behavior

image

image

Testing instructions

  • Apply D72672-code to your sandbox to ensure the theme demo API v1.1 is in your sandbox. Note that the mshot image might be broken as D72672-code should be shipped first.
  • Follow D71467-code to add your user id into the whitelist in your sandbox
  • Go to /start/setup-site/intent?siteSlug=<your_site>&flags=themes/premium
  • Select “build”
  • Check the premium themes are showing on the page and you can select that theme if the plan is premium or above
  • Check the premium themes won't show on the page if the plan is free or personal

Related to #59025

@matticbot
Copy link
Contributor

matticbot commented Dec 16, 2021

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

App Entrypoints (~565 bytes removed 📉 [gzipped])

name                   parsed_size           gzip_size
entry-gutenboarding        +3340 B  (+0.2%)    +1029 B  (+0.2%)
entry-domains-landing        -86 B  (-0.0%)     +158 B  (+0.1%)
entry-main                   +70 B  (+0.0%)      +95 B  (+0.0%)
entry-login                  +70 B  (+0.0%)      +97 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~31 bytes added 📈 [gzipped])

name             parsed_size           gzip_size
signup                 +55 B  (+0.0%)      +31 B  (+0.0%)
jetpack-connect        +55 B  (+0.0%)      +31 B  (+0.0%)
accept-invite          +55 B  (+0.0%)      +31 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 (~204 bytes added 📈 [gzipped])

name                                   parsed_size           gzip_size
async-load-signup-steps-design-picker       +926 B  (+0.9%)     +204 B  (+0.6%)

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.

@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch from 8e119eb to 5ca9b57 Compare December 17, 2021 08:27
@arthur791004 arthur791004 changed the title WIP Design Picker: Include premium themes Design Picker: Include premium themes Dec 17, 2021
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 17, 2021
@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch from 7ffdaaf to 6674bbd Compare December 17, 2021 10:38
return async ( dispatch ) => {
dispatch( { type: RECOMMENDED_THEMES_FETCH, filter } );
const query = {
search: '',
number: 50,
tier: '',
tier,
Copy link
Member

Choose a reason for hiding this comment

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

This means the fetching takes the tier into account, but the reducer doesn't take the tier into account.
https://github.com/Automattic/wp-calypso/blob/031a60d1891e2c610d9e8048a241c7e523d8cef0/client/state/themes/reducer.js#L491

It will stash the response in the store using only the filter, which means when the selector gets called it will return whatever tier just happens to have been last fetched.

A fix for this would be to store the response in the redux using both the filter and tier. But since I'd already planned on moving the theme fetching to @automattic/design-picker anyway, I thought the addition of premium themes would be a good opportunity to make that change. Making the code sharable with Gutenboarding rather than modifying the recommended themes reducer.

I'm making that change here: #59342

#59342 doesn't implement the actual feature that this PR does though i.e. it doesn't use hasFeature to decide whether to request premium themes or not. But I think that it'd be good to merge #59342 first so that we don't have to modify the Redux store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh cool, I don't notice that we use action.filter as the key of the themes. I think #59342 is a great refactor and I'm looking forward to merging it 👍

@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch from 6674bbd to 7c02162 Compare December 21, 2021 09:29
@arthur791004 arthur791004 changed the base branch from trunk to update/fetch-design-picker-with-react-query December 21, 2021 09:29
Base automatically changed from update/fetch-design-picker-with-react-query to trunk December 21, 2021 21:07
@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch from ab76d59 to 147c197 Compare December 22, 2021 02:35
Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

Just tested using Business plan, and planHasFeature() doesn't return true for FEATURE_PREMIUM_THEMES. Not sure why though.

return (
<Tooltip
position="top center"
// @ts-expect-error: @types/wordpress__components doesn't align with latest @wordpress/components
Copy link
Member

Choose a reason for hiding this comment

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

Much tidier 😅 I've worked around this in much hackier ways before


const [ userLoggedIn, isPremiumThemesAvailable ] = useSelector( ( state ) => [
isUserLoggedIn( state ),
planHasFeature( sitePlanSlug, FEATURE_PREMIUM_THEMES ),
Copy link
Member

Choose a reason for hiding this comment

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

planHasFeature() isn't actually a selector so doesn't need useSelector()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops...

@arthur791004
Copy link
Contributor Author

Just tested using Business plan, and planHasFeature() doesn't return true for FEATURE_PREMIUM_THEMES. Not sure why though.

Do you enable the feature flags? See:

isEnabled( 'themes/premium' ) ? FEATURE_PREMIUM_THEMES : null,

Or maybe your business plan is expired 🤔

@arthur791004 arthur791004 added the [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging label Jan 3, 2022
@a8ci18n
Copy link

a8ci18n commented Jan 3, 2022

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7071877

Thank you @arthur791004 for including a screenshot in the description! This is really helpful for our translators.

@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch 2 times, most recently from c387d06 to 04efb6e Compare January 7, 2022 09:43
Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

Do you enable the feature flags?

This was my problem, I'd forgotten the feature flag.

Looks good 👍
I've pushed some changes to the unit tests to fix them and to include a test for the case where the theme config is missing the stylesheet information.

The thumbnails didn't work, as expected. But I'm confident it'll work once the diff has shipped because the iframe'd preview of the design is working. I suggested some changes to the diff.

We have to wait for the backend to ship first anyway, but I think we don't need to feel blocked by the string freeze error. When you drill into the details of the translation task it's basically all done now.

@arthur791004
Copy link
Contributor Author

The thumbnails didn't work, as expected. But I'm confident it'll work once the diff has shipped because the iframe'd preview of the design is working. I suggested some changes to the diff.

Yep, I think the thumbnail will work after the backend is shipped!

@a8ci18n
Copy link

a8ci18n commented Jan 14, 2022

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7071877

Thank you @arthur791004 for including a screenshot in the description! This is really helpful for our translators.

@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch from 431b806 to cfd1008 Compare January 17, 2022 03:10
@autumnfjeld
Copy link
Contributor

autumnfjeld commented Jan 18, 2022

@arthur791004 Premium themes looks good.

  • I counted five premium themes in total (in the "Show All" category.)
  • I selected Byrne and it was successfully applied with starter content for a Home page with three posts.

Could we update the tooltip to be:
"Let your site stand out from the crowd with a modern and stylish Premium theme. Premium themes are included in your plan.”

image

Just to make sure I understand the order of operations in coordinating with the launch of the five Premium Themes (pdgA0m-b7-p2)

  • merge this PR, keeping the feature flag so that we(internally) have a way to view the "soft launched" Premium Themes
  • users(public) cannot see the "soft launched" Premium Themes
  • when Premium Themes are launched to the public they will be automatically visible to users, you will need to update this line .

Is that correct?

@arthur791004
Copy link
Contributor Author

arthur791004 commented Jan 18, 2022

Could we update the tooltip to be:
"Let your site stand out from the crowd with a modern and stylish Premium theme. Premium themes are included in your plan.”

Sure, I'll add the new copy. But we need to wait for the translation after we update the copy. Besides, we should keep in mind that we have to remove/replace “Premium themes are included in your plan.” when we work on showing premium themes for the user with free or personal plan.

Is that correct?

Yep, you're right! But I think we don't need to update this line. Is there any reason we have to?

@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch from 710c7d3 to 96ddc1c Compare January 18, 2022 06:16
@autumnfjeld
Copy link
Contributor

Yep, you're right! But I think we don't need to update this line. Is there any reason we have to?

Nope. All good. 😄

// @ts-expect-error: @types/wordpress__components doesn't align with latest @wordpress/components
delay={ 300 }
text={ `${ __(
'Let your site stand out from the crowd with a modern and stylish Premium theme.',
Copy link
Contributor

Choose a reason for hiding this comment

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

@arthur791004 Thanks for thinking ahead on this and making these two separate i18n strings! 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this string 'Let your site stand out from the crowd with a modern and stylish Premium theme.', already translated? If so, then don't let the new string block merging this PR.

We can add the new string in a subsequent PR. (I think translations can take a week or two right? )

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the first string is done: https://translate.wordpress.com/developer/arthur791004 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll add a new string in follow-up PR.

@arthur791004 arthur791004 force-pushed the feat/design-picker-premium-themes branch from 96ddc1c to 9a1f03c Compare January 20, 2022 04:57
@arthur791004 arthur791004 merged commit 4b4cae6 into trunk Jan 20, 2022
@arthur791004 arthur791004 deleted the feat/design-picker-premium-themes branch January 20, 2022 06:26
@github-actions github-actions bot removed [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 20, 2022
@@ -1,6 +1,7 @@
import { isEnabled } from '@automattic/calypso-config';
import DesignPicker, {
FeaturedPicksButtons,
PremiumBadge,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change triggers the following TypeScript error:

yarn tsc -p client/tsconfig.json --noEmit

client/landing/gutenboarding/onboarding-block/designs/index.tsx:1:24 - error TS2614: Module '"@automattic/design-picker"' has no exported member 'PremiumBadge'. Did you mean to use 'import PremiumBadge from "@automattic/design-picker"' instead?

1 import DesignPicker, { PremiumBadge, getAvailableDesigns } from '@automattic/design-picker';
                         ~~~~~~~~~~~~

Could you fix it @arthur791004? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Well apparently this is an issue with tsc, and it's just a matter of rebuilding packages to make those errors go away (more information here). Sorry for the ping Arthur.

@a8ci18n
Copy link

a8ci18n commented Jan 28, 2022

Translation for this Pull Request has now been finished.

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

Successfully merging this pull request may close these issues.

None yet

6 participants