Try and Customize WP.com theme on Jetpack. #10126

budzanowski · 2016-12-16T16:38:43Z

Info

Implements Try&Customize feature for Wpcom themes on Jetpack site.

Test

Open Jetpack site /design on calypso, click ellipsis menu and chose Try and Customize this should move user to try&customize page.

On internal falure this will show error banner to the user. @folletto - request for designer approval.

matticbot · 2016-12-16T16:38:45Z

Test live: https://calypso.live/?branch=add/try-and-customize-on-jetpack

ockham · 2016-12-16T17:17:38Z

Ideally, I'd like to avoid page and getState inside the action. The '...' menu actually supports action and getUrl in an option at the same time, see here. The main problem here is the async nature of the action...

seear · 2016-12-16T18:32:28Z

Ideally, I'd like to avoid page and getState inside the action.

The action could take a callback.

ockham · 2016-12-16T19:37:47Z

Okay, I guess we'll stick with the page call and the getState for now since I can't think of anything that makes this any better (callback would alter option shape for all other options inside theme-options, unless I'm reading you wrong, @seear).

ockham · 2016-12-16T19:44:47Z

client/state/themes/actions.js

+
+	return ( dispatch, getState ) => {
+		dispatch( {
+			type: THEME_TRY_AND_CUSTOMIZE_ON_JETPACK_REQUEST,


This is where I really wish we had a separate install action that we'd just use here to .then( page ( /*...*/ ), since the entire tryAndCustomizeWpcomThemeOnJetpack action is really just that. I'll provide some more rationale -- with the new middleware-based error handling (#10018), I currently have to listen to THEME_ACTIVATE_REQUEST_FAILURE, even though what I really am listening for is errors resulting from a failed install. This is because activateWpcomThemeOnJetpack dispatches those THEME_ACTIVATE_ actions even while it's installing.

Much rather than adding a listener for THEME_TRY_AND_CUSTOMIZE_ON_JETPACK_REQUEST on top of the one that listens to THEME_ACTIVATE_REQUEST_FAILURE while both are still really just listening to failed installation, I think we should finally introduce the installWpcomThemeOnJetpack action along with properly named action name constants, and compose both activateWpcomThemeOnJetpack and tryAndCustomizeWpcomThemeOnJetpack from it.

I think we should finally introduce the installWpcomThemeOnJetpack action along with properly named action name constants, and compose both activateWpcomThemeOnJetpack and tryAndCustomizeWpcomThemeOnJetpack from it.

Yeah, I would suggest that now we have the two lists and install working, we can take the time to do this properly.

ockham · 2016-12-16T20:09:38Z

client/state/themes/actions.js

+	//Add -wpcom suffix. This suffix tells the endpoint that we want to
+	//install WordPress.com theme. Without the suffix endpoint would look
+	//for theme in .org
+	const suffixedThemeId = themeId + '-wpcom';


I think we can actually keep this function generic -- maybe at some point we really want to install a theme from WP.org; we'll be able to re-use this function then. It's easy enough for the consumer to add the prefix themselves.

ockham · 2016-12-16T20:17:38Z

Suggested roadmap (see my inline comments for context):

Let's get Themes, Actions: Install WP.com theme on Jetpack action. #9924 in. That's the installTheme action along with tests and everything we could ever dream of. It seems like we're also going to need install state for UI purposes (for Try & Customize), too.
Let's rebase this one on it and implement tryAndCustomizeWpcomThemeOnJetpack as installTheme.then( page ( ) ), essentially. activateWpcomThemeOnJetpack will be installTheme and activateTheme.
Framework: bump localforage to 1.4.3 #10018 can then listen to THEME_INSTALL_REQUEST_FAILURE (instead of THEME_ACTIVATE_REQUEST_FAILURE which is misleading) to cover both cases.
Also, let's just call them installTheme (could be conceivably a WP.org theme too in the future), installAndActivate, and installAndTryAndCustomize.

budzanowski · 2016-12-19T13:10:31Z

Rebased.

ockham · 2016-12-19T18:12:09Z

client/my-sites/themes/theme-options.js

+	header: i18n.translate( 'Try & Customize on:', {
+		comment: 'label in the dialog for opening the Customizer with the theme in preview'
+	} ),
+	action: tryAndCustomizeWpcomThemeOnJetpack,


What about dropping the installWpcomThemeAndTryAndCustomize wrapper and doing

action: ( themeId, siteId ) => { installAndTryAndCustomize( themeId + '-wpcom', siteId ) }

here?

ockham · 2016-12-19T18:13:50Z

client/state/themes/actions.js

+function installAndTryAndCustomize( themeId, siteId ) {
+	return ( dispatch, getState ) => {
+		dispatch( {
+			type: THEME_TRY_AND_CUSTOMIZE_ON_JETPACK_REQUEST,


I'd go as far as considering dropping state handling (i.e. dispatching THEME_TRY_AND_CUSTOMIZE_ON_JETPACK_REQUEST* in this function) and really just compose it from installTheme and that page call. I don't think we lose any information that way.

budzanowski · 2017-01-04T13:36:34Z

Rebased.

ockham · 2017-01-04T23:27:10Z

client/state/themes/actions.js

+ * @param  {String}   siteId       Jetpack Site ID
+ * @return {Function}              Action thunk
+ */
+export function installWpcomThemeAndTryAndCustomize( themeId, siteId ) {


For consistency's sake, let's nix the wrapper and add the suffix in theme-options (like so)

ockham · 2017-01-04T23:28:22Z

client/state/themes/actions.js

@@ -404,6 +405,44 @@ export function clearActivated( siteId ) {
 }

 /**
+ * Triggers a network request to install WordPress.com theme on Jetpack site.


Can be .com/.org agnostic: let's s/WordPress.com/a/

ockham · 2017-01-04T23:30:51Z

client/my-sites/themes/theme-options.js

+		comment: 'label in the dialog for opening the Customizer with the theme in preview'
+	} ),
+	action: installWpcomThemeAndTryAndCustomize,
+	hideForSite: ( state, siteId ) => ! canCurrentUser( state, siteId, 'edit_theme_options' ),


I think we should also hide if ! isJetpackSite( state, siteId ), just to be sure.

ockham · 2017-01-04T23:37:45Z

client/state/themes/actions.js

+					return;
+				}
+				const url = getThemeCustomizeUrl( getState(), theme, siteId );
+				page( url );


If you want to modularize things, you could put the contents of the .then() statement into a separate action. That might simplify testing a bit.

ockham · 2017-01-04T23:42:42Z

client/state/themes/actions.js

+			.then( () => {
+				const theme = getTheme( getState(), siteId, themeId );
+				if ( ! theme ) {
+					return;


Do we want to dispatch a _FAILURE here (to show an error message, e.g. thru the notices middleware)?

(If we do dispatch a FAILURE, this should really be a separate action, see my other comment.)

seear · 2017-01-05T11:08:56Z

client/state/notices/middleware.js

@@ -175,6 +176,7 @@ export const handlers = {
 	[ PUBLICIZE_CONNECTION_UPDATE_FAILURE ]: onPublicizeConnectionUpdateFailure,
 	[ GUIDED_TRANSFER_HOST_DETAILS_SAVE_SUCCESS ]: dispatchSuccess( translate( 'Thanks for confirming those details!' ) ),
 	[ SITE_FRONT_PAGE_SET_FAILURE ]: dispatchError( translate( 'An error occurred while setting the homepage' ) ),
+	[ THEME_TRY_AND_CUSTOMIZE_FAILURE ]: dispatchError( translate( 'An error occured while trying to customize theme' ) ),


This could be shortened to 'Error customizing theme'.

We had a quick chat. Following the rule of thumb of always give an indication to the user on "what can I do now" instead of blunt error messages, let's go with:

"Error customizing theme, check with support"

budzanowski · 2017-01-05T12:22:20Z

Rebased.

folletto · 2017-01-05T13:39:28Z

Design wise: 👍

ockham · 2017-01-05T14:56:03Z

client/state/themes/actions.js

+	};
+}
+
+export function tryAndCustomize( themeId, siteId ) {


JSDoc pls? 😄 Sry for nagging, but it's the only function without one in this file, I think...

ockham · 2017-01-05T15:00:01Z

client/state/themes/test/actions.js

@@ -871,4 +874,144 @@ describe( 'actions', () => {
 			} );
 		} );
 	} );
+
+	describe( '#installAndTryAndCustomize', () => {


We've discussed this before, but would you consider changing this test to only check if installTheme and tryAndCustomizeTheme are dispatched (much like the installAndActivate test does)? I think that's the actual unit we're testing here. In addition, you could add a test just for tryAndCustomize that does the spying on page and covers the failure case.

(This way, we might get away without mockery and redux-mock-store.)

ockham · 2017-01-05T15:01:01Z

package.json

@@ -181,6 +181,7 @@
    "react-addons-test-utils": "15.4.0",
    "react-hot-loader": "1.3.0",
    "react-test-env": "0.2.0",
+    "redux-mock-store": "1.2.1",


If we decide to keep this, do we need to make shrinkwrap? (I'm not sure if it's required for devDependencies, but you could try and see if it makes a difference.)

ockham · 2017-01-05T15:29:24Z

Code looks good. Left a note on testing since I think the new unit test is a bit big and checks a bit more than it needs to.

budzanowski added the [Feature Group] Appearance & Themes Features related to the appearance of sites. label Dec 16, 2016

budzanowski added this to the Themes: JetPress Rally milestone Dec 16, 2016

budzanowski self-assigned this Dec 16, 2016

ockham reviewed Dec 16, 2016

View reviewed changes

ockham mentioned this pull request Dec 16, 2016

Themes, Actions: Install WP.com theme on Jetpack action. #9924

Merged

3 tasks

budzanowski force-pushed the add/try-and-customize-on-jetpack branch from 36a8963 to dbd96e5 Compare December 19, 2016 13:10

ockham reviewed Dec 19, 2016

View reviewed changes

hoverduck added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 21, 2016

ockham added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 3, 2017

budzanowski force-pushed the add/try-and-customize-on-jetpack branch from ac310a4 to 972a3f0 Compare January 4, 2017 13:34

ockham reviewed Jan 4, 2017

View reviewed changes

seear reviewed Jan 5, 2017

View reviewed changes

budzanowski added 4 commits January 5, 2017 13:04

Try and Customize WP.com theme on Jetpack.

56bd0ed

Make function generic.

7cfa3c1

Add think wrapper.

9c6e485

Simplify action implementation.

b3580b2

budzanowski added 12 commits January 5, 2017 13:10

Fix merge error.

194976b

Check if we have theme before we install.

5ead67f

Fix tests.

78e977d

Fix bug!

7c067ee

Correct tests after bugfix.

b19e67a

Don't use wrapper.

397c9d2

Make doc option agnostic.

586d3bc

Guard against non jetpack sites.

29d7eec

Move try and customize to separate action.

45030a0

Fix tests. Error in code.

5af83dc

Add error notice.

bebf837

Add error notice.

ed0cad6

budzanowski force-pushed the add/try-and-customize-on-jetpack branch from 35d6054 to ed0cad6 Compare January 5, 2017 12:21

folletto removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jan 5, 2017

ockham reviewed Jan 5, 2017

View reviewed changes

budzanowski added 4 commits January 6, 2017 13:15

Adjust test so they have more clear structure.

d95dab3

Not needed anymore.

9f21476

Add JSdoc

6b18168

Add check for FAILURE dispatch.

eac61d3

budzanowski merged commit cf66c9a into master Jan 6, 2017

budzanowski deleted the add/try-and-customize-on-jetpack branch January 7, 2017 12:04

lancewillett removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try and Customize WP.com theme on Jetpack. #10126

Try and Customize WP.com theme on Jetpack. #10126

budzanowski commented Dec 16, 2016 •

edited by artpi

Loading

matticbot commented Dec 16, 2016

ockham commented Dec 16, 2016

seear commented Dec 16, 2016

ockham commented Dec 16, 2016

ockham Dec 16, 2016

seear Dec 16, 2016

ockham Dec 16, 2016

ockham commented Dec 16, 2016

budzanowski commented Dec 19, 2016

ockham Dec 19, 2016

ockham Dec 19, 2016

budzanowski commented Jan 4, 2017

ockham Jan 4, 2017

ockham Jan 4, 2017

ockham Jan 4, 2017

ockham Jan 4, 2017

ockham Jan 4, 2017

seear Jan 5, 2017

folletto Jan 5, 2017

budzanowski commented Jan 5, 2017

folletto commented Jan 5, 2017

ockham Jan 5, 2017

ockham Jan 5, 2017

ockham Jan 5, 2017

ockham commented Jan 5, 2017

Try and Customize WP.com theme on Jetpack. #10126

Try and Customize WP.com theme on Jetpack. #10126

Conversation

budzanowski commented Dec 16, 2016 • edited by artpi Loading

Info

Test

matticbot commented Dec 16, 2016

ockham commented Dec 16, 2016

seear commented Dec 16, 2016

ockham commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Dec 16, 2016

budzanowski commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budzanowski commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budzanowski commented Jan 5, 2017

folletto commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Jan 5, 2017

budzanowski commented Dec 16, 2016 •

edited by artpi

Loading