Update fetchCurrentTheme, with proper naming and tests. #9419
Conversation
budzanowski
added
the
[Feature Group] Appearance & Themes
budzanowski
force-pushed
the
update/fetchCurrentTheme-refactore-and-testign
branch
from
c350cbf to
fd202cd
Compare
budzanowski
added
the
[Status] Needs Review
| export const THEME_RECEIVE_CURRENT = 'THEMES_RECEIVE_CURRENT'; | ||
| export const THEME_REQUEST_CURRENT = 'THEME_REQUEST_CURRENT'; | ||
| export const THEME_REQUEST_CURRENT_FAILURE = 'THEME_REQUEST_CURRENT_FAILURE'; | ||
| export const THEME_CURRENT_REQUEST_SUCCESS = 'THEME_CURRENT_REQUEST_SUCCESS'; |
There was a problem hiding this comment.
Nomenclature is hard 😄. This sounds a bit like it's the current request for a theme, not a request for the current theme. Also, to align with the new action name, it should be active, not current.
Soo... maybe ACTIVE_THEME_REQUEST etc? (I think it's fine to not prefix with THEME_.)
| @@ -75,26 +75,26 @@ export function incrementThemesPage( site ) { | |||
| }; | |||
| } | |||
|
|
|||
| export function fetchCurrentTheme( siteId ) { | |||
| export function requestActiveTheme( siteId ) { | |||
There was a problem hiding this comment.
Can we have a JSDoc block? 😄 Just once sentence should be enough (plus @param and @return).
| .reply( 404, failureResponse ); | ||
| } ); | ||
|
|
||
| it( 'should dispatch current theme request action when triggered', () => { |
There was a problem hiding this comment.
Let's stick to active theme for the nomenclature 😉 (same in the it statements below).
| export const THEME_REQUEST_CURRENT = 'THEME_REQUEST_CURRENT'; | ||
| export const THEME_REQUEST_CURRENT_FAILURE = 'THEME_REQUEST_CURRENT_FAILURE'; | ||
| export const ACTIVE_THEME_REQUEST_SUCCESS = 'ACTIVE_THEME_REQUEST_SUCCESS'; | ||
| export const ACTIVE_THEME_REQUEST = 'ACTIVE_THEME_REQUEST'; |
There was a problem hiding this comment.
Nitpick -- can we change the order here and define ACTIVE_THEME_REQUEST before ACTIVE_THEME_REQUEST_SUCCESS?
| @@ -75,26 +75,34 @@ export function incrementThemesPage( site ) { | |||
| }; | |||
| } | |||
|
|
|||
| export function fetchCurrentTheme( siteId ) { | |||
| /** | |||
| * This action queries wpcom endpoint for active theme for site. | |||
There was a problem hiding this comment.
Indentation -- there should be a space, not a tab before the * so it aligns with the first * in /**.
| export function fetchCurrentTheme( siteId ) { | ||
| /** | ||
| * This action queries wpcom endpoint for active theme for site. | ||
| * If request success formation about active theme is stored in Redux themes subtree. |
There was a problem hiding this comment.
Typo: should be information 😉
There was a problem hiding this comment.
Actually, should also be If request succeeds, ...
| THEME_REQUEST_CURRENT, | ||
| THEME_REQUEST_CURRENT_FAILURE, | ||
| ACTIVE_THEME_REQUEST_SUCCESS, | ||
| ACTIVE_THEME_REQUEST, |
There was a problem hiding this comment.
Let's import ACTIVE_THEME_REQUEST before ACTIVE_THEME_REQUEST_SUCCESS here, too.
|
Found couple more nits, but looking very good overall! Feel free to merge after you've addressed them 😄 |
matticbot
removed
the
[Status] Needs Review
budzanowski
deleted the
update/fetchCurrentTheme-refactore-and-testign
branch
Intoduction
Aim of this PR is to update fetchCurrentTheme action with naming that matches rest of Calypso and also adding unit tests that will help with further themes Redux subtree refactor.
3 Redux state tests are added in this PR:
To test:
/designin single site mode