Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

State: Migrate lib/post-formats to Redux state #8402

Merged
merged 9 commits into from
Oct 7, 2016
Merged

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Oct 3, 2016

This PR migrates the legacy lib/post-formats Flux store to Redux. It fixes #7925 and is part of the group effort to use a single data tree and query components throughout Calypso - #5046.

This PR introduces:

  • Action creators
  • Reducer
  • Selectors
  • Schema
  • Query components - QueryPostFormats

Tests are also included.

To test:

  • Run npm run test-client client/state/post-formats and verify all tests pass ✅

Implementation of this and removal of the old Flux store will be done in the subsequent PR(s).

/cc @aduth for review

@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components State labels Oct 3, 2016
@tyxla tyxla self-assigned this Oct 3, 2016
@matticbot
Copy link
Contributor

* Internal dependencies
*/
import { isValidStateWithSchema } from 'state/utils';
import * as schema from './schema';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably fine here since there's only a single export anyways, but in generally we should avoid import * as since it can't be optimized with future tree shaking in Webpack 2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. It's a good time to rename the exported schema object to something more unique anyway.

* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function requesting( state = {}, action ) {
Copy link
Contributor

@aduth aduth Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The createReducer utility can be handy to avoid some of the boilerplate associated with schema validation in reducers. By default, if there's no schema defined, it will always return initial state, else perform the validation for you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nifty, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if it was intentionally not used for the requesting reducer, but the nice thing is if you use it without a schema, it will return the default initial state for you, so this could be simplified to:

export const requesting = createReducer( {}, {
    [ POST_FORMATS_REQUEST ]: ( state, { siteId } ) => ( { ...state, [ siteId ]: true } ),
    [ POST_FORMATS_REQUEST_SUCCESS ]: ( state, { siteId } ) => ( { ...state, [ siteId ]: false } ),
    [ POST_FORMATS_REQUEST_FAILURE ]: ( state, { siteId } ) => ( { ...state, [ siteId ]: false } )
} );

additionalProperties: false,
patternProperties: {
// ID of the post format
'^[0-9a-z]+$': {
Copy link
Contributor

@aduth aduth Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a little too strict. Can post formats have dashes or underscores in their name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch there. Post formats are actually taxonomy terms, and their ID is actually the term slug. Term slugs are limited by sanitize_title_with_dashes(), which means they can also contain dashes and underscores.

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 3, 2016
@tyxla
Copy link
Member Author

tyxla commented Oct 4, 2016

@aduth Thanks for the review - I've addressed your suggestions and this is ready for a new review.

@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Oct 4, 2016
* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function requesting( state = {}, action ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if it was intentionally not used for the requesting reducer, but the nice thing is if you use it without a schema, it will return the default initial state for you, so this could be simplified to:

export const requesting = createReducer( {}, {
    [ POST_FORMATS_REQUEST ]: ( state, { siteId } ) => ( { ...state, [ siteId ]: true } ),
    [ POST_FORMATS_REQUEST_SUCCESS ]: ( state, { siteId } ) => ( { ...state, [ siteId ]: false } ),
    [ POST_FORMATS_REQUEST_FAILURE ]: ( state, { siteId } ) => ( { ...state, [ siteId ]: false } )
} );

}
}, postFormatsItemsSchema );

export default combineReducers( {
Copy link
Contributor

@aduth aduth Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to be active in Redux state, we need to include the default reducer in the combined reducer of client/state/index.js .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I was thinking to do this in the subsequent PR where we implement this into the components, but it's fine to do it here as well.

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 5, 2016
@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Oct 6, 2016
@tyxla
Copy link
Member Author

tyxla commented Oct 6, 2016

@aduth I think we should be ready to go, unless you have other suggestions.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@@ -65,10 +66,11 @@ export const reducer = combineReducers( {
pageTemplates,
plugins,
plans,
preferences,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the order 👍

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 6, 2016
@tyxla tyxla force-pushed the add/redux-post-formats branch from 41d7712 to df79fd8 Compare October 7, 2016 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

State: Migrate lib/post-formats to Redux state
3 participants