Theme Sheet: Use information from WordPress.org for themes on Jetpack sites #9875

ockham · 2016-12-07T18:32:48Z

Allows us to find out if a theme on a Jetpack site is present on WP.org and act accordingly.

Show the tags in the features, but unlinked? ✅
Remove download as it would imply "download from Jetpack site" and doesn't seem entirely accurate to me. -- I've kept it for now since I thought it might still be useful -- my counter-example being a Jetpress site that was recently transferred and is using a WPCOM theme the user might also want to use elsewhere. Oh well, a bit contrived, I guess 😄
You might also like: could [s]tay, but let's get without it for now. ✅
Support...
- If it's a theme on .org, link to the theme's specific support forum (example: https://wordpress.org/support/theme/twentysixteen )
- if it's not a theme on .org (i.e. custom, premium, etc.), hide the Support link

(Question about item 4. -- I'm not sure I've implemented it correctly, I'm currently not removing the entire 'Support' tab but only the link to the theme forum. I wasn't sure I understood the idea correctly, and I thought the other two links might still make some sense even on a JP site. Clarification needed 😄)

To do all that, this PR:

Adds a fetchThemeInformation method to lib/wporg
Changes requestTheme to hit that endpoint if siteId === 'wporg', thus putting themes fetched from there in a new wporg subtree
Adds a normalization util to account for different attribute names in objects obtained from the WP.org API
Changes the getTheme selector so that if a theme on a JP site is requested, supplementary data (upon availability) such as demo URI, download URI, or taxonomies is merged into the returned object.
Adds an isWporgTheme( state, themeId ) aliasing !! getTheme( state, 'wpcom', themeId )
Modifies the theme sheet to conditionally hide some UI elements (such as the 'Open Live Demo' link) if some given theme attributes aren't available (for cases where no data from WP.org was available).

To test:

Try the theme sheet on a Jetpack site, ideally for a theme that's not on WPCOM. Try two different themes, one that is on WP.org, and one that isn't.
WP.org theme: Check that the following UI elements are present and work properly:
- 'Open Live Demo' link
- 'Download' button
- Theme Features (but they don't link anywhere!)
- Related theme suggestions are not there
- 'Visit forum' link in the 'Support' tab should point to theme-specific forum on WP.org
non-WP.org theme: None of the above UI elements should be present
Also check that wpcom theme sheets still work as before.

(For reference, here's an example response containing all the fields we can get from WP.org: https://api.wordpress.org/themes/info/1.1/?action=theme_information&request%5Bslug%5D=twentyfourteen)

matticbot · 2016-12-07T18:32:49Z

Test live: https://calypso.live/design?branch=add/theme-info-fetching-from-wporg

ockham · 2016-12-08T23:22:27Z

client/components/data/query-active-theme/index.jsx

@@ -12,10 +12,7 @@ import { isRequestingActiveTheme } from 'state/themes/selectors';

 class QueryActiveTheme extends Component {
 	static propTypes = {
-		siteId: PropTypes.oneOfType( [
-			PropTypes.number,
-			PropTypes.oneOf( [ 'wpcom' ] )


A theme can only be active on an actual site, not on all of WP.com 😄

(Discovered while adding wporg to some other propTypes and schemas.)

seear · 2016-12-09T13:25:39Z

We may want to hide the Features block if it is empty:

budzanowski

Code wise looks good, although hard to follow: to much stuff is going on at the same time. 👍

seear

Code looks good. I've tested with both wporg and custom themes on a Jetpack site.

Only glaring thing is the empty Features panel.

seear · 2016-12-09T13:43:04Z

client/my-sites/theme/main.jsx

+		// * If it's a premium theme
+		// * If it's on a Jetpack site, and the theme object doesn't have a 'download' attr
+		//   Note that not having a 'download' attr would be permissible for a theme on WPCOM
+		//   since we don't provide any for some themes found on WordPress.org (notably the 'Twenties').


(notably the 'Twenties')

Think it is just Twenty Sixteen

I checked a bunch of other Twenties to which the same seems to apply, but possibly not all of them.

seear · 2016-12-09T14:20:49Z

client/my-sites/theme/main.jsx

@@ -451,6 +469,7 @@ const ThemeSheet = React.createClass( {
 		return (
 			<Main className="theme__sheet">
 				<QueryTheme themeId={ this.props.id } siteId={ siteIdOrWpcom } />
+				{ isJetpack && <QueryTheme themeId={ this.props.id } siteId="wporg" /> }


Do we need the two instances of <QueryTheme/>?

Yeah, the first one fetches the theme object from the JP site, and the second one fetches the supplementary one from the WP.org API.

seear · 2016-12-09T14:33:06Z

client/state/themes/test/actions.js

 		context( 'with a wpcom site', () => {
+			useNock( ( nock ) => {
+				nock( 'https://public-api.wordpress.com:443' )
+					.persist()


I don't think these persist calls are generally necessary unless you want the same result multiple times.

Breaks tests if I remove it for all wpcom, wporg, and JP 😕

ockham · 2016-12-09T15:30:23Z

Code wise looks good, although hard to follow: to much stuff is going on at the same time. 👍

Yeah, sorry about that. I guess I could've split it into even more PRs, but then again, I already did that twice, so I wanted to get it done...

…available

…JP sites)

… endpoint

… test

…Id key There's no such thing as an active theme on all of WP.com :-)

ockham · 2016-12-09T17:41:39Z

We may want to hide the Features block if it is empty

Done in 8cbee7e

seear

…mes on JP sites

ockham · 2016-12-09T18:20:28Z

(Mini-fix to schema.js to get rid of a state validation warning.)

omarjackman · 2016-12-10T01:59:07Z

client/my-sites/theme/main.jsx

 			const term = isValidTerm( item.slug ) ? item.slug : `feature:${ item.slug }`;
 			return (
 				<li key={ 'theme-features-item-' + item.slug }>
-					<a href={ `/design/filter/${ term }/${ siteSlug || '' }` }>{ item.name }</a>
+					{ isJetpack
+						? <a>{ item.name }</a>


Just curious if adding the <a> is actually doing anything here. Are styles applied to it because its an anchor?
Maybe it could just be { item.name }

Yeah, we're styling it. (I had removed it and noticed it didn't look the same.) I opted for keeping the element instead of tweaking the styling 😄

folletto · 2016-12-12T12:56:05Z

Remove download as it would imply "download from Jetpack site" and doesn't seem entirely accurate to me. -- I've kept it for now since I thought it might still be useful -- my counter-example being a Jetpress site that was recently transferred and is using a WPCOM theme the user might also want to use elsewhere. Oh well, a bit contrived, I guess

Keep it only if it's already working flawlessly. If you need even just 10 minutes of extra work to make it work across .com/Jetpack (how does Jetpack zip the theme to allow its download?) remove it. It's not a priority right now.

Question about item 4. -- I'm not sure I've implemented it correctly, I'm currently not removing the entire 'Support' tab but only the link to the theme forum. I wasn't sure I understood the idea correctly, and I thought the other two links might still make some sense even on a JP site. Clarification needed 😄

Ok, let me branch it out! :)

For Jetpack:

If it's a local / uploaded / custom theme → the Support tab disappears as we can't provide support from .com, nor we know if there's a .org forum.
If it's a Business User → show the tab with only renderContactUsCard ("Need extra help?")

ockham · 2016-12-12T13:12:04Z

Keep it only if it's already working flawlessly. If you need even just 10 minutes of extra work to make it work across .com/Jetpack (how does Jetpack zip the theme to allow its download?) remove it. It's not a priority right now.

Oh, maybe I wasn't quite clear -- we're only showing the Download link if the theme is found on WP.org, and has a download URI (pointing to a ready-made zip, that is) already. Otherwise we're hiding it.

folletto · 2016-12-12T13:12:56Z

Oh, maybe I wasn't quite clear -- we're only showing the Download link if the theme is found on WP.org, and has a download URI (pointing to a ready-made zip, that is) already. Otherwise we're hiding it.

Ok cool, I thought the whole "check .org's availability" was postponed. :)

Excellent then! :D

ockham · 2016-12-12T13:15:29Z

For Jetpack:

Ok cool, I thought the whole "check .org's availability" was postponed. :)

Hehe, no, that's what this PR does 😄 (cf. #9869 (comment)):

I thought initially that the is-on-dotorg check might be a bit too difficult to implement given that we're on a rather tight schedule, but some tinkering (#9875) revealed that it's probably not that hard after all.

ockham · 2016-12-12T13:15:49Z

If it's a local / uploaded / custom theme → the Support tab disappears as we can't provide support from .com, nor we know if there's a .org forum.

Oh, I was trying to implement #9869 (comment)

My suggestion would be:

If it's a theme on .org, link to the theme's specific support forum (example: https://wordpress.org/support/theme/twentysixteen )

So if you look at the 'Support' tab's forum link for a theme on a JP site that's also found on WP.org, it will point to that theme's WP.org support forum right now.

folletto · 2016-12-12T13:18:01Z

So if you look at the 'Support' tab's forum link for a theme on a JP site that's also found on WP.org, it will point to that theme's WP.org support forum right now.

Same as the above. I was working on the assumption .org's checking wasn't included right now, in that case:

If there is a .org forum, show its block.
If there is Business Plan enabled, show its block.
If the Support tab is empty (none of the above) hide it.

ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels Dec 7, 2016

ockham added this to the Themes: JetPress Rally milestone Dec 7, 2016

ockham self-assigned this Dec 7, 2016

ockham mentioned this pull request Dec 7, 2016

Theme Sheet: Fix for Jetpack sites #9869

Merged

ockham force-pushed the add/theme-info-fetching-from-wporg branch 4 times, most recently from ae46bfe to f9ac0e8 Compare December 8, 2016 13:56

ockham changed the title ~~Themes: Enable fetching of theme information from WordPress.org~~ Theme Sheet: Use information from WordPress.org for themes on Jetpack sites Dec 8, 2016

ockham 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 Dec 8, 2016

ockham commented Dec 8, 2016

View reviewed changes

budzanowski approved these changes Dec 9, 2016

View reviewed changes

seear approved these changes Dec 9, 2016

View reviewed changes

ockham added 3 commits December 9, 2016 17:01

lib/wporg: Add fetchThemeInformation() function

de72945

state/themes/actions: Enable fetching of theme information from wporg

f15e9b5

Theme Sheet: On JP sites, try querying theme information from wporg

b4b6fcc

ockham force-pushed the add/theme-info-fetching-from-wporg branch 2 times, most recently from 839faf9 to 5404e83 Compare December 9, 2016 16:03

ockham added 2 commits December 9, 2016 17:04

state/themes/utils: Add normalizeWporgTheme() util

6205d1b

state/theme/actions: Normalize wporg themes

5d99f41

ockham force-pushed the add/theme-info-fetching-from-wporg branch 2 times, most recently from 4eedad4 to 4276754 Compare December 9, 2016 16:48

ockham added 3 commits December 9, 2016 18:23

state/themes/selectors#getTheme(): On JP sites, merge WP.org data if …

8a13f1c

…available

Theme Sheet: Only show 'Open Live Demo' link when we have a Demo URL

0c70ad7

Themes: Use getThemeForumUrl() selector instead of getForumUrl() helper

26ec67e

ockham added 11 commits December 9, 2016 18:24

Theme Sheet: Hide forum link for non-wporg themes on JP sites

5a08543

state/themes/selectors: Have getTheme use taxonomies from wporg (for …

5a9185b

…JP sites)

Theme Sheet: Don't link feature tags on JP sites

6c80ebc

state/themes/actions: Fix one requestTheme() test to really mimic the…

23f07df

… endpoint

state/themes/actions: Add tests for requestTheme() with a Jetpack site

f80aab5

state/themes/actions: Add tests for requestTheme() with the WP.org API

9a54916

state/test/utils: Add tags/theme_feature check to normalizeWporgTheme…

4cd7918

… test

QueryTheme: Accept 'wporg' as possible siteId propType

157a342

state/themes/schema: Accept 'wporg' as possible siteId key

2b0a677

state/themes/schema: Don't accept wpcom as possible activeThemes site…

443292e

…Id key There's no such thing as an active theme on all of WP.com :-)

QueryActiveTheme: Don't allow 'wpcom' as a siteId prop

738baab

ockham force-pushed the add/theme-info-fetching-from-wporg branch from 6e80e0f to 738baab Compare December 9, 2016 17:24

Theme Sheet: Hide Features Card if no features are found

8cbee7e

seear approved these changes Dec 9, 2016

View reviewed changes

state/themes/schema: stylesheet and demo_uri aren't mandatory for the…

62b6788

…mes on JP sites

ockham added 2 commits December 9, 2016 19:22

state/themes/schema: author_uri isn't mandatory for WP.org themes

42429c5

state/themes/actions: Guard agains false return value from WP.org API

1834607

omarjackman reviewed Dec 10, 2016

View reviewed changes

ockham merged commit 47f1a19 into master Dec 12, 2016

ockham deleted the add/theme-info-fetching-from-wporg branch December 12, 2016 12:48

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

okmttdhr mentioned this pull request Aug 2, 2023

Remove unused methods in Theme Detail page #80129

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme Sheet: Use information from WordPress.org for themes on Jetpack sites #9875

Theme Sheet: Use information from WordPress.org for themes on Jetpack sites #9875

ockham commented Dec 7, 2016 •

edited

Loading

matticbot commented Dec 7, 2016 •

edited by folletto

Loading

ockham Dec 8, 2016

seear commented Dec 9, 2016

budzanowski left a comment

seear left a comment

seear Dec 9, 2016

ockham Dec 9, 2016

seear Dec 9, 2016

ockham Dec 9, 2016

seear Dec 9, 2016

ockham Dec 9, 2016

ockham commented Dec 9, 2016

ockham commented Dec 9, 2016

seear left a comment

ockham commented Dec 9, 2016

omarjackman Dec 10, 2016

ockham Dec 10, 2016

folletto commented Dec 12, 2016

ockham commented Dec 12, 2016

folletto commented Dec 12, 2016

ockham commented Dec 12, 2016

ockham commented Dec 12, 2016

folletto commented Dec 12, 2016

Theme Sheet: Use information from WordPress.org for themes on Jetpack sites #9875

Theme Sheet: Use information from WordPress.org for themes on Jetpack sites #9875

Conversation

ockham commented Dec 7, 2016 • edited Loading

matticbot commented Dec 7, 2016 • edited by folletto Loading

Choose a reason for hiding this comment

seear commented Dec 9, 2016

budzanowski left a comment

Choose a reason for hiding this comment

seear left a 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

Choose a reason for hiding this comment

ockham commented Dec 9, 2016

ockham commented Dec 9, 2016

seear left a comment

Choose a reason for hiding this comment

ockham commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

folletto commented Dec 12, 2016

ockham commented Dec 12, 2016

folletto commented Dec 12, 2016

ockham commented Dec 12, 2016

ockham commented Dec 12, 2016

folletto commented Dec 12, 2016

ockham commented Dec 7, 2016 •

edited

Loading

matticbot commented Dec 7, 2016 •

edited by folletto

Loading