Themes: Use Selectors instead of Helpers #6924

ockham · 2016-07-20T17:05:21Z

Add a connectOptions helper function that bundles all binding to state and site (if given) in one place for greater ease-of-use, and have it use selectors instead of helpers for getUrl attrs. Furthermore, use (newly written) isActiveTheme selector in theme options' hideForTheme predicates.

The latter feature is particularly relevant as it is the first (and hardest) step to decoupling theme objects from contextual data, and using selectors like isActiveTheme that take state, site ID, and theme as arguments instead tying that information to theme object (as in theme.active). This will allow us to keep only one list of all wpcom themes that just contains bare, i.e. non-contextual, theme information, without relying on attrs like .active or .purchased, that we can use both in multi- and single-site mode (instead of multiplicating that list for all sites a user has).

We can't use the isThemePurchased selector yet as getSitePurchases (the selector it relies on) seems to be buggy -- it doesn't always return the same results. We'll need to do a separate PR -- the code changes to use that selector in hideForTheme should be fairly easy, most of the effort will need to go into researching why and how the underlying selector is broken.

This PR also removes shouldComponentUpdates in the Theme and ThemesList components. This is required so those components also update if buttonOptions change, which, as of this PR, will happen e.g. if a theme is activated, since ThemeOptions now makes use of the isActiveTheme selector inside hideForTheme.

Note that I had to add a new isActive selector which relies on the existing current-theme Redux subtree instead of the isThemeActive selector that relies on a given site's options.theme_slug attr (and which I hoped would allow us to stop tracking active themes per site in a separate subtree). However, there's a rather tricky bug with options.theme_slug which is documented in a comment in the source code, so I'm afraid we'll have to keep our own current-theme subtree around for the time being (though we should refactor it to be in line with the new themes state tree structure). cc @budzanowski

No visual changes. To test:
Make sure that the theme showcase still works as before (compare to master or production).

Since this is an invasive change, it requires extensive testing in all modes (logged-out, multi-site, single-site [for both wpcom and Jetpack sites], and theme sheets)
Testing should cover:
Verifying that a theme thumbnail's ellipsis menu contains all the right items, and that they all work as expected.
Verifying that call-to-action buttons in a theme sheet, and in theme preview are present, present the correct action, and are correctly labelled.

Next step -- next PR:
Replace all remaining occurrences of theme.active by the new isActiveTheme selector. Figure out what to do about the Theme component.

TODO for later PRs:

Have selectors accept a themeID instead of an entire theme object (and have them obtain other required theme attributes as needed). (Possibly same for site -> siteId)
Replace all get...Url() helpers by selectors.
Make isPremiumTheme a selector
Get rid of window dependency
Add tests for theme-options, move to lib/ or components/.
Add getCustomizeUrl( state, siteId ) alias for getThemeCustomizeUrl( state, null, siteId ) ?
Use our new selectors to implement this one
Delete helpers.js (once all usages have been replaced by selectors)

Supersedes #6165.

Test live: https://calypso.live/?branch=update/themes-turn-helpers-into-selectors-take-two

ockham · 2016-07-29T16:55:27Z

Themes: Turn helpers into selectors

lancewillett · 2016-10-10T18:32:03Z

Hi @ockham Checking in on this older pull request. Still in progress?

ockham · 2016-10-12T13:58:54Z

Stalled right now, will probably reboot after our current milestone is done.

seear · 2016-11-07T15:13:41Z

Saw this on calypso.live when switching sites, but can't reproduce:

seear

Looks good, just a couple of questions.

Tested in particular:

Current site bar when switching sites
Options for various showcase pages
Activating a theme from 'all sites' page

seear · 2016-11-07T14:44:38Z

client/my-sites/theme/main.jsx

+	// defaultOption prop. Otherwise, there will be an error that will prevent the
+	// state update from finishing properly, hence not updating defaultOption at all.
+	// The solution to this incredibly intricate issue is simple: Give ThemeSheet
+	// a valid defaultProp for defaultOption.


This is a pretty long comment. Should maybe be in the docs instead?

Sounds like a good idea. Which ones, though? 😄

Heh, good question. I was thinking client/my-sites/theme/README.md. Or at least make it a multiline comment.

Part of it is quite general (the stuff about this being a react-redux quirk), but since the rest is really quite specific to the code here (much more than general README notes), I'm inclined to leave it here. Will turn it into a multi-line /* comment */ tho.

seear · 2016-11-07T15:19:39Z

client/my-sites/themes/multi-site.jsx

-	}
+export default ( props ) => (
+	<MultiSiteThemeShowcase { ...props }
+		options={ [


Now much easier to see what the options will be for the different showcase pages 👍

seear · 2016-11-07T15:31:49Z

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

+
+export const connectOptions = connect(
+	( state, { options: optionNames, site } ) => {
+		let options = pick( ALL_THEME_OPTIONS, optionNames );


what does this line give us over using the passed in options directly?

The idea is that individual theme option objects -- used separately from connectOptions -- almost don't make any sense at all, since they'd always need to be bound at least to state (and dispatch, in case of e.g. activate -- and install and delete, later on), and sometimes to siteId. So I'm thinking of option objects more of an implementation detail right now (the interface being connectOptions). I should probably stop exporting them.

(Expanding on that, we could consider turning those strings into constants so that one would have to import { THEME_OPTION_ACTIVATE } from 'theme-options'; but so far, i haven't quite felt like typing so much...)

seear · 2016-11-07T15:42:13Z

client/components/theme/index.jsx

@@ -62,10 +61,6 @@ const Theme = React.createClass( {
 		actionLabel: React.PropTypes.string
 	},

-	shouldComponentUpdate( nextProps ) {
-		return ! isEqual( nextProps.theme, this.props.theme );


This came in as an optimization #2915

Is it safe to remove?

The main rationale for this is that we now have to re-render if the buttonOptions prop is changed, which will happen e.g. if a different theme is activated: In some of the options' hideForTheme predicates, we're using the isActiveTheme selector. So if those predicates change, we definitely want to re-render, or a newly activated theme will have the wrong items in its '...' menu. The same holds in principle true for the screenshotClickUrl and onScreenshotClick.

I think we mostly need to make sure that buttonOptions don't change unnecessarily, and possibly by adding back shouldComponentUpdate, with a deep comparison (i.e. another isEqual) of buttonOptions alongside the one for theme, plus some comparisons for the stuff I mentioned above (screenshotClickUrl, onScreenshotClick, etc).

I think we mostly need to make sure that buttonOptions don't change unnecessarily, and possibly by adding back shouldComponentUpdate, with a deep comparison (i.e. another isEqual) of buttonOptions alongside the one for theme, plus some comparisons for the stuff I mentioned above (screenshotClickUrl, onScreenshotClick, etc).

In this PR or a separate one? I don't want to hold this one up—on the other hand I'm loath to introduce a performance regression.

seear · 2016-11-07T21:28:03Z

client/components/theme/index.jsx

@@ -63,7 +62,12 @@ const Theme = React.createClass( {
 	},

 	shouldComponentUpdate( nextProps ) {
-		return ! isEqual( nextProps.theme, this.props.theme );
+		// TODO: Once we're not using theme.active and theme.purchased anymore, just compare theme.id instead of entire theme objects.
+		return ! isEqual( this.props.theme, this.props.theme ) ||


this.props.theme twice here

Argh. Good catch, ty!

Add a `connectOptions` helper function that bundles all binding to state and site (if given) in one place for greater ease-of-use, and have it use selectors instead of helpers for `getUrl` attrs. Furthermore, use (newly written) `isActiveTheme` selector in theme options' `hideForTheme` predicates. The latter feature is particularly relevant as it is the first (and hardest) step to decoupling theme objects from contextual data, and using selectors like `isActiveTheme` that take state, site ID, and theme as arguments instead tying that information to theme object (as in `theme.active`). This will allow us to keep only one list of all wpcom themes that just contains bare, i.e. non-contextual, theme information, without relying on attrs like `.active` or `.purchased`, that we can use both in multi- and single-site mode (instead of multiplicating that list for all sites a user has). We can't use the `isThemePurchased` selector yet as `getSitePurchases` (the selector it relies on) seems to be buggy -- it doesn't always return the same results. We'll need to do a separate PR -- the code changes to use that selector in `hideForTheme` should be fairly easy, most of the effort will need to go into researching why and how the underlying selector is broken. This PR also removes `shouldComponentUpdate`s in the `Theme` and `ThemesList` components. This is required so those components also update if `buttonOptions` change, which, as of this PR, will happen e.g. if a theme is activated, since `ThemeOptions` now makes use of the `isActiveTheme` selector inside `hideForTheme`. Note that I had to add a new `isActive` selector which relies on the existing `current-theme` Redux subtree instead of the `isThemeActive` selector that relies on a given site's `options.theme_slug` attr (and which I hoped would allow us to stop tracking active themes per site in a separate subtree). However, there's a rather tricky bug with `options.theme_slug` which is documented in a comment in the source code, so I'm afraid we'll have to keep our own `current-theme` subtree around for the time being (though we should refactor it to be in line with the new `themes` state tree structure). No visual changes. To test: Make sure that the theme showcase still works as before (compare to `master` or production). * Since this is an invasive change, it requires extensive testing in all modes (logged-out, multi-site, single-site [for both wpcom and Jetpack sites], and theme sheets) * Testing should cover: * Verifying that a theme thumbnail's ellipsis menu contains all the right items, and that they all work as expected. * Verifying that call-to-action buttons in a theme sheet, and in theme preview are present, present the correct action, and are correctly labelled.

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

ockham added this to the Themes: Maintenance milestone Jul 20, 2016

ockham self-assigned this Jul 20, 2016

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch from aed54a7 to 53bd88d Compare July 20, 2016 17:46

ockham mentioned this pull request Jul 29, 2016

Themes: Turn helpers into selectors #6165

Closed

4 tasks

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch 3 times, most recently from 998338e to 2a1ca9a Compare August 2, 2016 11:10

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch from b5a4a7c to 8ccbd57 Compare August 8, 2016 22:36

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch 3 times, most recently from 2c4830e to 4a12b3d Compare October 24, 2016 13:17

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch from 4a12b3d to 96cac05 Compare October 25, 2016 12:31

ockham changed the title ~~Themes: Turn helpers into selectors, take two~~ Themes: Use Selectors instead of Helpers Oct 25, 2016

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch 6 times, most recently from 02c40fd to be74822 Compare October 29, 2016 19:19

ockham mentioned this pull request Oct 29, 2016

My Sites Sidebar: Use getThemeCustomizeUrl() selector instead of helper #9022

Merged

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch from be74822 to 78d4fef Compare October 29, 2016 19:27

This was referenced Oct 29, 2016

Themes Thanks Modal: Use selectors for details and customize URLs #9023

Merged

Related Themes Card: Use getThemeDetailsUrl() selector instead of helper #9024

Merged

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch 2 times, most recently from 8911e20 to 9e27889 Compare October 29, 2016 20:00

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch from 9e27889 to 961fd34 Compare October 29, 2016 20:03

This was referenced Oct 29, 2016

Theme Actions: Use siteId instead of site object #9025

Closed

Themes: Re-organize Redux Subtree #8785

Merged

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 Nov 5, 2016

seear approved these changes Nov 7, 2016

View reviewed changes

seear reviewed Nov 7, 2016

View reviewed changes

This was referenced Nov 7, 2016

Themes: Use isActiveTheme selector instead of theme.active attr #9193

Merged

Refactored activateTheme #9181

Merged

ockham force-pushed the update/themes-turn-helpers-into-selectors-take-two branch from 4181ad1 to 7e78fe5 Compare November 8, 2016 13:02

ockham merged commit f3ca699 into master Nov 8, 2016

ockham deleted the update/themes-turn-helpers-into-selectors-take-two branch November 8, 2016 13:45

ockham mentioned this pull request Nov 8, 2016

Revert "Themes: Use Selectors instead of Helpers" #9214

Merged

mattwondra mentioned this pull request Nov 10, 2016

Themes: Use isThemePurchased selector instead of theme.purchased attr #9263

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes: Use Selectors instead of Helpers #6924

Themes: Use Selectors instead of Helpers #6924

ockham commented Jul 20, 2016 •

edited

Loading

ockham commented Jul 29, 2016

lancewillett commented Oct 10, 2016

ockham commented Oct 12, 2016

seear commented Nov 7, 2016

seear left a comment

seear Nov 7, 2016

ockham Nov 7, 2016

seear Nov 7, 2016

ockham Nov 7, 2016

seear Nov 7, 2016

seear Nov 7, 2016

ockham Nov 7, 2016 •

edited

Loading

ockham Nov 7, 2016

seear Nov 7, 2016

ockham Nov 7, 2016

seear Nov 7, 2016

seear Nov 7, 2016

ockham Nov 7, 2016

Themes: Use Selectors instead of Helpers #6924

Themes: Use Selectors instead of Helpers #6924

Conversation

ockham commented Jul 20, 2016 • edited Loading

ockham commented Jul 29, 2016

lancewillett commented Oct 10, 2016

ockham commented Oct 12, 2016

seear commented Nov 7, 2016

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 Nov 7, 2016 • edited Loading

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 Jul 20, 2016 •

edited

Loading

ockham Nov 7, 2016 •

edited

Loading