Themes: Re-organize Redux Subtree #8785

ockham · 2016-10-17T15:54:38Z

This is the capstone of the Themes Redux State Tree Refactor. See PRs referenced below for a selection of related PRs. It changes the Theme Showcase to use the new <QueryThemes /> component and selectors corresponding to the new themes.queries subtree (instead of the old <ThemesListFetcher /> and friends).

The idea for the new arch is far-reaching de-duplication, following best Redux practices as seen in other parts of Calypso.

This means that there's one per-site list of themes objects functioning as the single source of truth; data from different endpoints (think theme details) will be merged into those authoritative theme objects. We try to avoid storing site-specific data inside the theme objects though.
All new selectors accept theme IDs as an arg, not entire theme objects. Instead, they look up relevant theme attributes via the new getTheme() selector.

TODO (later PRs):

Check if we need more checks like hasFeature( FEATURE_UNLIMITED_PREMIUM_THEMES ), e.g. for a8c-stickered sites (they're showing themes with prices on this branch right now!)

To test:

Analytics
SSR

matticbot · 2016-10-17T15:54:39Z

Test live: https://calypso.live/design?branch=update/themes-redux-subtree

ockham · 2016-11-03T11:53:35Z

client/state/themes/test/actions.js

+		} );
+	} );
+
+	describe( '#athemeActivationFailed()', () => {


Typo: athemeActivationFailed

ockham · 2016-11-04T11:33:12Z

client/state/themes/actions.js

@@ -133,32 +136,30 @@ export function receiveServerError( error ) {
 * Returns an action object to be used in signalling that a theme activation
 * has been triggered
 *
- * @param  {Object}  theme Theme received
- * @param  {Object}  site Site used for activation
+ * @param  {Number}  themeId Theme to be activated


themeIds are strings like twentysixteen or mood :-)

(There are a couple more instances of this JSDoc argument type in this commit that need fixing as well.)

ockham · 2016-11-04T11:34:10Z

client/state/themes/actions.js

 * @return {Object}  Action object
 */
-export function activateTheme( theme, site ) {
+export function themeActivation( themeId, siteId ) {


What's the reason for the rename? I'd rather stick with the verb-like activateTheme.

Ah, probably so it's more in line with themeActivated and stuff like themeActivationFailed? Well what about themeActivate and themeActivateFailure? (failure -- being a noun -- is more consistent with success too, and also with action name constants).

themeActivation is just action creator.
activateTheme is still chosen name for function that activates themes 😄

Action names will still be renamed to have _REQUST part.
As for the naming I have actually struggled a bit.
Naming for function is problematic here as actions and functions using them have logically similar meaning.
So maybe activateTheme for function triggering activation, and a verbose version for action creator activateThemeRequest ( activateThemeRequestSuccess ... )?

themeActivation is just action creator.
activateTheme is still chosen name for function that activates themes 😄

Ah right 😄 Well if themeActivation is really just a simple AC and only used from activateTheme, what about just inlining it?

ockham · 2016-11-04T11:40:29Z

client/state/themes/actions.js

+	search_term: null
+};
+
+export function activateTheme( themeId, siteId, trackThemesActivationData = defaultTrackData ) {


I don't think the default really makes a lot of sense here since we'll always be passing a param for tracking here. If it's just for testing, I think we should rather create a dummy object in tests.

That was only for testing. No default value is good because it will force devs to fill this in when they will want to use activate.

ockham · 2016-11-04T19:46:23Z

client/state/themes/test/actions.js

+				.post( '/rest/v1.1/sites/2211667/themes/mine', { theme: 'badTheme' } )
+				.reply( 404, {
+					error: 'unknown_theme',
+					message: 'Unknown theme'


Nitpick -- I just used the developer console to see what I'd really get, and the actual error and message are theme_not_found and The specified theme was not found, respectively.

I have assumed same values as in requestTheme. Corrected.

ockham · 2016-11-04T19:51:59Z

I'm liking what I see :-) I'm starting to realize how distracting the sheer size of this PR is, so I'm wondering -- should we try and rip out the new activation related actions into a separate PR so we can land those earlier?

budzanowski · 2016-11-07T10:40:53Z

I would leave that decision to you. It is hard for me to judge if this changes can land on their own. They would still need reducers updated to match, and probably couple of more places. For sure it is better to split if it is possible

ockham · 2016-11-07T11:32:23Z

Yeah, I think I'm pretty sure we should split it out, and I think the changes it is going to require are doable. (This PR here OTOH is otherwise going to be a nightmare to review.)

Instead of THEMES_RECEIVE_SERVER_ERROR. Drop the latter.

… to getThemesLastPageForQuery

…LastPageForQuery

ockham added the [Feature Group] Appearance & Themes Features related to the appearance of sites. label Oct 17, 2016

ockham added this to the Themes: JetPress Rally milestone Oct 17, 2016

ockham self-assigned this Oct 17, 2016

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 17, 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 Oct 17, 2016

ockham mentioned this pull request Oct 20, 2016

Suggestion: Use Immutables .withMutations for better performance. #8773

Closed

ockham force-pushed the update/themes-redux-subtree branch from 3bb0124 to 3236c1c Compare October 20, 2016 15:57

ockham force-pushed the update/themes-redux-subtree branch from 3236c1c to b5fd268 Compare October 28, 2016 19:25

ockham commented Nov 3, 2016

View reviewed changes

ockham commented Nov 4, 2016

View reviewed changes

ockham mentioned this pull request Nov 4, 2016

Theme Showcase: Prevent re-fetching themes that are already fetched server-side #9104

Closed

ockham commented Nov 4, 2016

View reviewed changes

budzanowski added a commit that referenced this pull request Nov 7, 2016

Import activateTheme() from #8785

c98b1e2

budzanowski mentioned this pull request Nov 7, 2016

Refactored activateTheme #9181

Merged

5 tasks

budzanowski added a commit that referenced this pull request Nov 9, 2016

Import activateTheme() from #8785

ef90b98

budzanowski added a commit that referenced this pull request Nov 10, 2016

Import activateTheme() from #8785

c5b1e32

ockham force-pushed the update/themes-redux-subtree branch 7 times, most recently from 09e64f2 to cbd7095 Compare November 18, 2016 16:20

ockham and others added 15 commits December 14, 2016 19:09

state/themes/reducer: Wire queryRequestErrors reducer

6f1a179

Themes SSR: In logged-out test, use THEMES_REQUEST_FAILURE

ebebe0a

Instead of THEMES_RECEIVE_SERVER_ERROR. Drop the latter.

state/themes/reducer: Wire lastQuery reducer

a6ec637

ThemeSheet: Use isThemePremium selector instead of old isPremium util

8df2324

Theme Sheet: Remove some obsolete mocks from test

687cc2e

Themes: Reset page on query change

7bf53b2

ThemesSelection: s/isActiveTheme/isThemeActive/g

fd868ea

Theme Sheet: Update isLoaded() guard for new Redux arch

0310599

Themes Selection: Add back placeholders

17f58d6

Fix Jetpack pagination errors.

38d3e8d

state/themes/actions: Normalize Jetpack themes

4feab2a

state/themes/utils: Another JP filtering fix

ee80b43

state/themes/selectors: Fix isThemesLastPageForQuery tests

f5dacbc

state/themes/selectors: Wrap themes lists in uniq() calls

6591a89

ThemesSelection: Remove obsolete selector

af73a2a

ockham force-pushed the update/themes-redux-subtree branch from a8699c4 to af73a2a Compare December 14, 2016 18:09

ockham added 3 commits December 14, 2016 19:21

state/themes/selectors#isThemesLastPageForQuery: Move isJetpack logic…

578a295

… to getThemesLastPageForQuery

state/themes/selectors Fix tests

362fd05

state/themes/selectors: Add tests to cover JP case for [get|is]Themes…

358437f

…LastPageForQuery

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 14, 2016

ockham merged commit 782f545 into master Dec 14, 2016

ockham deleted the update/themes-redux-subtree branch December 14, 2016 20:34

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 14, 2016

ockham mentioned this pull request Dec 14, 2016

Theme Showcase: Remove a number of obsolete props #10061

Merged

ockham mentioned this pull request Jan 18, 2017

Themes: Current Theme Bar and Theme Sheet Broken for Retired Theme #10760

Closed

This was referenced Feb 21, 2017

Themes Selectors: Don't convert from Immutable.js toJS() #1599

Closed

Themes: De-duplicate active theme information #7167

Closed

ockham mentioned this pull request Mar 16, 2017

Themes: Clean up Redux state #4057

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes: Re-organize Redux Subtree #8785

Themes: Re-organize Redux Subtree #8785

ockham commented Oct 17, 2016 •

edited

Loading

matticbot commented Oct 17, 2016 •

edited by folletto

Loading

ockham Nov 3, 2016

ockham Nov 4, 2016

ockham Nov 4, 2016

ockham Nov 4, 2016

ockham Nov 4, 2016

budzanowski Nov 4, 2016

budzanowski Nov 4, 2016

ockham Nov 4, 2016

ockham Nov 4, 2016

budzanowski Nov 7, 2016

ockham Nov 4, 2016

budzanowski Nov 7, 2016

ockham commented Nov 4, 2016

budzanowski commented Nov 7, 2016

ockham commented Nov 7, 2016

Themes: Re-organize Redux Subtree #8785

Themes: Re-organize Redux Subtree #8785

Conversation

ockham commented Oct 17, 2016 • edited Loading

matticbot commented Oct 17, 2016 • edited by folletto 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

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

budzanowski commented Nov 7, 2016

ockham commented Nov 7, 2016

ockham commented Oct 17, 2016 •

edited

Loading

matticbot commented Oct 17, 2016 •

edited by folletto

Loading