Themes: sheet Support logic changes to include Jetpack #11294

folletto · 2017-02-09T14:06:54Z

This PR reviews the logic behind the "Support" tab in theme sheets, following the logic from #11263:

Contact us — shown only if “Paid” and “Not Jetpack”
Theme forum (Premium) — shown only if “Premium theme”
Theme forum (general) — shown only if “.com theme” and “not Premium theme”
CSS Support — shown only if “.com theme”

To test:

Please note there's a bug 🐞 currently that won't show the "Theme Forums" if the theme sheet isn't for a URL with a site (single site). Check everything else (update: fix in #11296).

Check one of each of the kinds above:

Open /theme/textbook/support/$site (Free theme, Paid user) → should show "Need extra help?" + "Have a question (volounteers)?" + "Need CSS help?"
Open /theme/shoreditch/support (Free theme, Free user) → should show "Need CSS help?" only
Open /theme/shoreditch/support (Free theme, Paid user) → should show "Need extra help?" + "Need CSS help?"
Open /theme/shoreditch/support/$jetpack (Free theme, JP site) → should show "Need CSS help?" only.
Open /theme/$customtheme/support/$jetpack (Custom theme, JP site) → should show only the forum link for the custom theme

This list is probably not exhaustive, but should cover all the major cases.

matticbot · 2017-02-09T14:06:55Z

Test live: https://calypso.live/design?branch=add/theme-sheet-jp-help

seear · 2017-02-09T15:42:12Z

Theme forum (Premium) — shown only if “Premium theme”
Theme forum (general) — shown only if “.com theme” and “not Premium theme”

Since there is no premium theme that is not also a .com theme, seems like we should show the forum for any .com theme, which gives us just this logic for the forum:

{ forumUrl && isWpcomTheme &&
    this.renderSupportThemeForumCard( buttonCount++ ) }

seear · 2017-02-09T15:48:22Z

Seems that logic inside the getThemeForumUrl() selector needs a look. I don't think that it should be changing its return value based on whether a site is jetpack anymore.

edit: link

wp-calypso/client/state/themes/selectors.js

Line 474 in b23a70a

export function getThemeForumUrl( state, themeId, siteId ) {

seear · 2017-02-09T15:26:16Z

client/my-sites/theme/main.jsx

@@ -644,6 +643,7 @@ export default connect(
 	( state, { id } ) => {
 		const selectedSite = getSelectedSite( state );
 		const siteSlug = selectedSite ? getSiteSlug( state, selectedSite.ID ) : '';
+		const isJetpack = selectedSite && isJetpackSite( state, selectedSite.ID );


We don't need the intermediate const here because the value is not used in this func. We can add it directly to the returned object below. Also, we should add isJetpack to the propTypes array at the start of the component.

seear · 2017-02-09T15:32:21Z

client/my-sites/theme/main.jsx

-				</div>
-			);
-		}
+		let buttonCount = 1;


We should destructure the props into locals to give more concise code below, like this:

const { isCurrentUserPaid, isJetpack, forumUrl, isPremium, isWpcomTheme } = this.props;

folletto · 2017-02-10T18:45:06Z

Since there is no premium theme that is not also a .com theme, seems like we should show the forum for any .com theme, which gives us just this logic for the forum:

Yeah true, I copied 1:1 the explanatory logic, but since it's the card that deals with the difference, no need there. Thanks!

Seems that logic inside the getThemeForumUrl() selector needs a look. I don't think that it should be changing its return value based on whether a site is jetpack anymore.

Seems ok to me... as a theme might be a custom one, so not a .org one, thus it might have no forum to refer to. Regardless, should it be a separate issue from this one?

We should destructure the props

We don't need the intermediate const

Done ✓ ✓

folletto · 2017-02-10T19:00:50Z

I also added a special case for logged out:

Ok it's ready for another code review. :)

Why? More granular conditions, supports properly Jetpack / uploaded themes

Why? What if you see the support tab while logged out? Let's mention what they are going to get if they start using our services.

seear · 2017-02-13T12:20:26Z

client/my-sites/theme/main.jsx

-					{ this.renderCssSupportCard() }
+					{ isCurrentUserPaid && ! isJetpack &&
+						this.renderSupportContactUsCard( buttonCount++ ) }
+					{ forumUrl && isWpcomTheme &&


Do we need the isWpcomTheme check here? Currently I see nothing at all for custom themes on jetpack sites:

That's... correct in the sense that we aren't really able to provide support.

Maybe we can just see if there's a URL? Yes it might make sense, as it should point elsewhere if it's not a .com theme...

folletto · 2017-02-13T13:16:17Z

Ok fixed the condition on forumUrl — I also pushed a change that doesn't show the Support tab if it's empty. The conditions are 1:1 all the conditions form the other function, in a line. I'm not sure if it's a better way to do it, but the fact it's the same series without simplification should help maintenance later.

Ready for another review :)

seear · 2017-02-13T14:17:39Z

client/my-sites/theme/main.jsx

 		const validSections = [];
 		validSections.push( '' ); // Default section
 		this.props.supportDocumentation && validSections.push( 'setup' );
-		validSections.push( 'support' );
+		( ! isLoggedIn || ( isCurrentUserPaid && ! isJetpack ) || forumUrl || isWpcomTheme ) && validSections.push( 'support' );


( ! isLoggedIn || ( isCurrentUserPaid && ! isJetpack ) || forumUrl || isWpcomTheme )

This logic seems a little fragile. Any time someone makes a tweak to the support UI logic below, they would have to know to change this line as well. Also this won't stop inbound links to /support.

Maybe we could have a default support message when none of the other cases apply?

Yes that's the reason why I'm a bit uncomfortable with that.

UX wise it's better overall to hide it — but ok, let me thing as a backup solution if I can come up with a message.

folletto · 2017-02-13T17:52:32Z

Ok, I replaced the Support tab hiding logic with this message:

Ready for another round. :)

seear

Ok, good to ship 👍

folletto added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels Feb 9, 2017

folletto added this to the Themes: JetPress Rally milestone Feb 9, 2017

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

folletto self-assigned this Feb 9, 2017

seear suggested changes Feb 9, 2017

View reviewed changes

folletto added 4 commits February 13, 2017 11:56

Theme: sheet "Support" cards logic restructured

e192241

Why? More granular conditions, supports properly Jetpack / uploaded themes

Theme: sheet "Support" primary button logic more explicit

9c1b123

Fix: cleared logic, syntax

e9a0e0d

Theme: added logged out support message

4522e21

Why? What if you see the support tab while logged out? Let's mention what they are going to get if they start using our services.

seear force-pushed the add/theme-sheet-jp-help branch from 87da4bb to 4522e21 Compare February 13, 2017 12:15

seear reviewed Feb 13, 2017

View reviewed changes

folletto added 2 commits February 13, 2017 13:12

Theme: simplified support logic for forum card

cc49380

Themes: if support tab is empty, let's not show it

de6e404

seear reviewed Feb 13, 2017

View reviewed changes

Theme: instead of hiding Support tab, show message

27e5209

seear approved these changes Feb 13, 2017

View reviewed changes

folletto merged commit f10af25 into master Feb 14, 2017

folletto deleted the add/theme-sheet-jp-help branch February 14, 2017 10:54

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

folletto mentioned this pull request Feb 14, 2017

Themes: sheet Support logic changes to include Jetpack #11263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes: sheet Support logic changes to include Jetpack #11294

Themes: sheet Support logic changes to include Jetpack #11294

folletto commented Feb 9, 2017 •

edited

Loading

matticbot commented Feb 9, 2017 •

edited by folletto

Loading

seear commented Feb 9, 2017

seear commented Feb 9, 2017 •

edited

Loading

seear Feb 9, 2017

seear Feb 9, 2017

folletto commented Feb 10, 2017

folletto commented Feb 10, 2017 •

edited

Loading

seear Feb 13, 2017

folletto Feb 13, 2017

folletto commented Feb 13, 2017 •

edited

Loading

seear Feb 13, 2017

folletto Feb 13, 2017

folletto commented Feb 13, 2017 •

edited

Loading

seear left a comment

Themes: sheet Support logic changes to include Jetpack #11294

Themes: sheet Support logic changes to include Jetpack #11294

Conversation

folletto commented Feb 9, 2017 • edited Loading

To test:

matticbot commented Feb 9, 2017 • edited by folletto Loading

seear commented Feb 9, 2017

seear commented Feb 9, 2017 • edited Loading

seear Feb 9, 2017

Choose a reason for hiding this comment

seear Feb 9, 2017

Choose a reason for hiding this comment

folletto commented Feb 10, 2017

folletto commented Feb 10, 2017 • edited Loading

seear Feb 13, 2017

Choose a reason for hiding this comment

folletto Feb 13, 2017

Choose a reason for hiding this comment

folletto commented Feb 13, 2017 • edited Loading

seear Feb 13, 2017

Choose a reason for hiding this comment

folletto Feb 13, 2017

Choose a reason for hiding this comment

folletto commented Feb 13, 2017 • edited Loading

seear left a comment

Choose a reason for hiding this comment

folletto commented Feb 9, 2017 •

edited

Loading

matticbot commented Feb 9, 2017 •

edited by folletto

Loading

seear commented Feb 9, 2017 •

edited

Loading

folletto commented Feb 10, 2017 •

edited

Loading

folletto commented Feb 13, 2017 •

edited

Loading

folletto commented Feb 13, 2017 •

edited

Loading