Themes: Refactor themes-selection

[ockham]

tl;dr: Instead of passing down a gradually more bound getButtonOptions() function, invoke it fully inside main.jsx already, and make its getUrl and action properties functions that accept theme as an argument. Then, invoke those functions inside the Theme and MoreButton components, i.e. only at the very bottom of the component hierarchy. Also, use the return value from getButtonOptions() to pass some other props more cleanly. Why? Been an illegible mess on the brink of unmaintenable.

---

Removes the theme argument from getButtonOptions(), as returned from theme-options. This way, getButtonOptions() returns an object that is much easier to work with than the bound functions it previously returned. Instead, that object's (nested) getUrl and action properties are now bound functions that accept theme and are invoked at the very leaves of the component hierarchy -- in the 'More' button menu, when the latter's entries are rendered. (If this turns out to affect performance negatively, we might want to tackle #1599 as a follow-up to this PR.)

This means no more expensive JSX .bind()s along that chain. It also means we have to pass an entire theme object down to the 'More' button, instead of just select attributes, as getUrl and action functions rely on helpers that use all different theme object properties. Note that we also had a hack in place that re-created a theme object from individual properties, and that this PR now also gets rid of.

Furthermore, that bundled information on theme actions is used inside main.jsx to pass data more cleanly down to descendant components. For example, action and label for the theme screenshot are now set there (instead of passing down customize and togglePreview actions as props individually, and letting the Theme component itself decide on the labelling. Ugh.).

No visual changes. To test -- check that the theme showcase works as before. Also check the logged-out signup flow since it also uses ThemesList.

Some more possible refactoring items for future PRs:

• For components that are meant for use as elements in a list (such as Theme inside ThemesList), avoiding JSX .bind()s quite naturally leads to a pattern where the list (parent) component passes two different kind of data to the element (child) component: 1 -- the individual element data (the theme object in this case), and 2 -- a bunch of functions that accept the element data as an argument (e.g. onScreenshotClick), and are invoked by the element component. However, this is still a bit inconsistent as of this PR, as some of those functions are already invoked at ThemesList's level.
• Instead of passing actions to theme-options, we might import the actions module there, and only pass dispatch from main.
• We might want to give ThemesSiteSelectorModal an isVisible prop (instead of conditionally rendering it via { this.isThemeOrActionSet() && <ThemesSiteSelectorModal ... /> }). There used to be some issues with this approach in the past (as ThemesSiteSelectorModal renders Theme, so possibly due to missing props), but we might want to re-visit this.
• One neat way to assign screenshot action/label/URL at Theme component level would be by just picking the appropriate action object from buttonOptions, i.e. by passing the action name (e.g. 'preview') as a string for key lookup. This would somewhat minimize duplicated props. (For the special case of Theme components that don't need a more button menu (signup flow!), we could pass just one option in buttonOptions.) However, we can't currently do this due to different analytics events that we are recording.
• We are using names like getButtonOptions, getOptions, buttonContents etc for essentially the same thing, so we should probably settle on a unique name for these things.
• To get rid of some redundant logic, we might want to use theme-options for CurrentThemeCard (we can re-use isHidden in order to hide the 'Support' button on Jetpack sites, and for free themes).
• We might want a cleaner approach to tracking.
• WebPreview (and how its Button is wired to an action and given a label) could still use some more makeover.

[seear]

Interesting, this PropType is too complex to repeat in various places. Should we factor out this PropType somewhere so it can be shared? Might be better just to have it in one place and expect a PropType of object elsewhere.

[mcsf]

Yeah, I'd like to see a Calypso lib of PropTypes somewhere. Maybe something following the client/state hierarchy?

[ockham]

Can we use lodash's merge or similar here?

[ockham]

Can we use lodash's merge or similar here?

62b7335

[seear]

I've rebased to master to do some Chrome Timeline runs.

[seear]

Running the Chrome Timeline, performance between this branch and master look to be roughly comparable, with slow frames of similar duration and frequency.

This PR shows encouraging signs in terms of which functions are most expensive, with renderTheme and getButtonOptions dropping out of the list of worst offenders.

(Compare the percentages between these two shots, not the times)

This Branch

Master

[ockham]

@seear Great, thanks for looking into this!

[seear]

Code looks good to me after the walkthrough session and subsequent tweaks, so I'll give a 👍

I have not run any kind of rigorous testing, so it might be worth running either the e2e tests or going through a full set of manual tests before merge if you haven't already done so.

[ockham]

Thanks @seear!

Squashed, and ran e2e tests. Will merge once CircleCI finished.