Themes, Actions: Install WP.com theme on Jetpack action. #9924
Conversation
budzanowski
added
the
[Feature Group] Appearance & Themes
budzanowski
added
the
[Status] Needs Review
| * @param {Object} action Action payload | ||
| * @return {Object} Updated state | ||
| */ | ||
| export function installThemeOnJetpackRequests( state = {}, action ) { |
There was a problem hiding this comment.
This reducer indicates whether install is in progress or not, so probably needs the word progress in it.
There was a problem hiding this comment.
I can change it to have word progress - I have no preferences here :) I made it this way to follow general pattern of naming reducers we have in this file:
themeRequests , activeThemeRequests and couple of others indicate if the "progress" is ongoing but don't have word progress in theme.
There was a problem hiding this comment.
I made it this way to follow general pattern of naming reducers we have in this file
ok, that makes sense.
| case THEME_INSTALL_ON_JETPACK_REQUEST_FAILURE: | ||
| return Object.assign( {}, state, { | ||
| [ action.siteId ]: Object.assign( {}, state[ action.siteId ], { | ||
| [ action.themeId ]: THEME_INSTALL_ON_JETPACK_REQUEST === action.type |
There was a problem hiding this comment.
Currently, we would need to know the id of the theme to determine whether a site is currently installing something. Maybe themeId should be a value rather than a key?
There was a problem hiding this comment.
As we discussed: we should store full relation site - theme - bool. We rely on selectors to figure out proper answer to different type of questions like for example; is there an install process for site ongoing or what themes are being installed right now.
budzanowski
force-pushed
the
add/install-action-wpcom-theme-on-jetpack
branch
from
e0dc375 to
7b9197a
Compare
|
|
||
| return wpcom.undocumented().installThemeOnJetpack( siteId, wpcomThemeId ) | ||
| .then( ( ) => { | ||
| dispatch( { |
There was a problem hiding this comment.
We could dispatch receiveTheme with the API result here if we want.
There was a problem hiding this comment.
I am adding it but It looks like we do only receive limited subset of theme data from the endpoint.
| error | ||
| } ); | ||
| // push the error forward so connected then() can catch it. | ||
| throw error; |
There was a problem hiding this comment.
Maybe we shouldn't throw here at all? I think the idea is that we're already catching it above and handling it as graceful as possible by dispatching THEME_INSTALL_ON_JETPACK_REQUEST_FAILURE. Cf other network request... actions.
| @@ -509,6 +509,9 @@ export const THEME_ACTIVATE_REQUEST_FAILURE = 'THEME_ACTIVATE_REQUEST_FAILURE'; | |||
| export const THEME_REQUEST = 'THEME_REQUEST'; | |||
| export const THEME_REQUEST_FAILURE = 'THEME_REQUEST_FAILURE'; | |||
| export const THEME_REQUEST_SUCCESS = 'THEME_REQUEST_SUCCESS'; | |||
| export const THEME_INSTALL_ON_JETPACK_REQUEST = 'THEME_INSTALL_ON_JETPACK_REQUEST'; | |||
There was a problem hiding this comment.
This is a nitpick but we should keep this list alphabetized 😄
ockham
force-pushed
the
add/install-action-wpcom-theme-on-jetpack
branch
from
5946b8f to
7b62f5c
Compare
|
Rebased. |
ockham
force-pushed
the
add/install-action-wpcom-theme-on-jetpack
branch
from
590f7d3 to
12c6d10
Compare
|
Renamed and polished some stuff as per #10126 (comment). We'll implement |
matticbot
removed
the
[Status] Needs Review
Info
Implements action creator tor trigger install endpoint. Requires implementation from #9923.
Implements install tracking reducer.
Testing
Not connected yet.
TODO