Editor: Use Redux state for Post Formats #8593

tyxla · 2016-10-07T15:50:01Z

This PR is a continuation of #8402, which migrated the legacy lib/post-formats Flux store to Redux. This PR:

Modifies the post formats to use the new Redux state
ES6ifies the editor post formats components
Fixes several ESLint warnings.
Removes the unused legacy PostFormatsStore Flux store.
Removes the unused legacy PostFormatsData data component.

To test:

Checkout this branch
Go to http://calypso.localhost:3000/post/$site/, where $site is a site supports one or more additional post formats.
Create a new post with a different post format and save it.
Open the edit page of the newly created post.
In all of the above cases, play around with the post formats and verify they work as expected (change, save and retrieve accordingly).

matticbot · 2016-10-07T15:50:03Z

Test live: https://calypso.live/?branch=update/post-formats-use-redux-state

tyxla · 2016-10-10T12:55:31Z

/cc @aduth

youknowriad

+84/-441 This is awesome! I look forward deleting all these Flux stores :). The code looks good, aside from the error I've got.

youknowriad · 2016-10-10T13:37:09Z

client/post-editor/editor-post-formats/accordion.jsx

-		postFormats: React.PropTypes.arrayOf( React.PropTypes.shape( {
-			slug: React.PropTypes.string,
-			label: React.PropTypes.string
-		} ) )


I suppose the change of the postFormats type is due to the return value of the getPostFormats selector. I was wondering whether we should add something like Object.values(postFormats) inside the selector to get an array instead of an object.

I know the object can have better performance when accessing postFormats by slug, but it feels more "natural" to have an array of postFormats when we call a function called getPostFormats, don't you think ?

it feels more "natural" to have an array of postFormats when we call a function called getPostFormats, don't you think ?

I don't actually 😃 That's why I converted it to an object in the Redux version. I really don't understand why it would be better to have that as an array. Array here just adds additional overhead, while object helps us simplify the code for managing the formats and accessing them.

I respect your point of view and It's a good compromise for me If you want to keep this part as is. but I can explain more my point of view.

I think storing it as an object in Redux is great, because we can write performant selectors based on this. We can write getPostFormats selector which we could call if we want to have the full list (array) while we can write another selector getPostFormat(state, slug) to access directly the required post format by Slug. Maybe I would also add another selector called getPostFormatsBySlug which returns the object by slug.

Thanks for sharing your view. It's always beneficial to hear another opinion. 👍

And regarding the getPostFormatBySlug / getPostFormat selector - I was thinking about writing one, but it wont be used anywhere, so we can live without it at this time. And considering that post formats are not a feature that is really evolving in the WordPress core, I doubt we'll expand our need for additional post format selectors anytime soon.

I'm fairly certain I've implemented selectors with both of these return value options in the past, and while it depends on the UI requirements, I'd probably side more with @youknowriad on this one, which is that:

The reducer shape tracks an object of post formats, keyed by slug

The selectors transform this to whatever is most appropriate for the UI

I think it's rare that the UI requirements would dictate returning the original object from the selector.

Here, I see two requirements:

Showing the label of the selected format in the accordion subtitle

Showing a list of all post format options

To the first, we could achieve this with a selector getPostFormat( state, siteId, slug ) to retrieve the single post format ~~object~~ label (instead of trying to look it up from the entire set as has been done 'til now, which can be error prone and is nice to have a tested selector for).

To the second, since it's used as a single unit (list), an array seems to be the most appropriate return value from getPostFormats( state, siteId ).

Also, tying this to my earlier comment, if we separate the requirements of the accordion and accordion contents where each has its own connect selector, you'll see that the distinction becomes very clear that getPostFormat for the current selected post value is most appropriate for the <EditorPostFormatsAccordion /> component and the getPostFormats for the <EditorPostFormats /> component (to list options).

Now, all this being said, these are things that we can incrementally refactor toward 😄

I realized pretty late in reviewing this that the object structure of the reducer is slug to label string, not slug to an object of properties, and while that makes it a little less obvious, I'd still stand by my previous remarks. Interesting discussion in any case 😄

I agree we can build up such changes incrementally - personally I'm not seeing any blockers here. After all the main purpose of this PR is to transfer the existing post formats components to Redux - further improvements can be done in subsequent PRs. But please feel free to correct me if you feel I'm wrong @aduth.

youknowriad · 2016-10-10T13:50:31Z

client/post-editor/editor-post-formats/index.jsx

+import FormRadio from 'components/forms/form-radio';
+import Gridicon from 'components/gridicon';
+import PostActions from 'lib/posts/actions';
+import stats from 'lib/posts/stats';


I noticed an error while testing.

I think the fix is something like import * as stats from 'lib/posts/stats' here. or better import only the functions you need recordEvent ect..

Good catch, fixing now.

tyxla · 2016-10-10T14:28:49Z

Thanks for the review @youknowriad! Feel free to let me know if you have any other suggestions.

aduth · 2016-10-10T19:15:41Z

client/post-editor/editor-drawer/index.jsx

 		return (
-			<PostFormatsData siteId={ this.props.site.ID }>
+			<div>
+				<QueryPostFormats siteId={ this.props.site.ID } />


We should strive for components to be self-sufficient with query components and connect applied as close to the UI requirements as possible. To that point, at the very least it should be the <EditorPostFormatsAccordion /> component which renders the <QueryPostFormats /> and retrieves postFormats from state. I might go one step further and say that both <EditorPostFormatsAccordion /> and <EditorPostFormats /> should separately connect to pull from postFormats state so that they're not so tightly coupled.

In the end, I'd love if <EditorDrawer /> were simply a render function like:

function EditorDrawer() { return ( <div className="editor-drawer"> <EditorDrawerTaxonomies /> <EditorDrawerFeaturedImage /> <EditorDrawerPageOptions /> <EditorDrawerSharing /> <EditorDrawerPostFormats /> <EditorDrawerSeo /> <EditorDrawerMoreOptions /> </div> ); }

Where each component is responsible for deciding itself whether to render and how.

That's a good point. I've now decoupled both <EditorPostFormats /> and <EditorPostFormatsAccordion />, so they separately connect and pull from the postFormats state. A good pro of this is that we no longer need to call the query component within the editor drawer.

aduth · 2016-10-10T19:24:03Z

client/post-editor/editor-post-formats/accordion.jsx

-		postFormats: React.PropTypes.arrayOf( React.PropTypes.shape( {
-			slug: React.PropTypes.string,
-			label: React.PropTypes.string
-		} ) )


I'm fairly certain I've implemented selectors with both of these return value options in the past, and while it depends on the UI requirements, I'd probably side more with @youknowriad on this one, which is that:

The reducer shape tracks an object of post formats, keyed by slug

The selectors transform this to whatever is most appropriate for the UI

I think it's rare that the UI requirements would dictate returning the original object from the selector.

Here, I see two requirements:

Showing the label of the selected format in the accordion subtitle

Showing a list of all post format options

To the first, we could achieve this with a selector getPostFormat( state, siteId, slug ) to retrieve the single post format ~~object~~ label (instead of trying to look it up from the entire set as has been done 'til now, which can be error prone and is nice to have a tested selector for).

To the second, since it's used as a single unit (list), an array seems to be the most appropriate return value from getPostFormats( state, siteId ).

aduth · 2016-10-10T19:26:26Z

client/post-editor/editor-post-formats/accordion.jsx

-		postFormats: React.PropTypes.arrayOf( React.PropTypes.shape( {
-			slug: React.PropTypes.string,
-			label: React.PropTypes.string
-		} ) )


Also, tying this to my earlier comment, if we separate the requirements of the accordion and accordion contents where each has its own connect selector, you'll see that the distinction becomes very clear that getPostFormat for the current selected post value is most appropriate for the <EditorPostFormatsAccordion /> component and the getPostFormats for the <EditorPostFormats /> component (to list options).

aduth · 2016-10-10T19:26:56Z

client/post-editor/editor-post-formats/accordion.jsx

-		postFormats: React.PropTypes.arrayOf( React.PropTypes.shape( {
-			slug: React.PropTypes.string,
-			label: React.PropTypes.string
-		} ) )


Now, all this being said, these are things that we can incrementally refactor toward 😄

aduth · 2016-10-10T19:32:51Z

client/post-editor/editor-post-formats/accordion.jsx

-		postFormats: React.PropTypes.arrayOf( React.PropTypes.shape( {
-			slug: React.PropTypes.string,
-			label: React.PropTypes.string
-		} ) )


I realized pretty late in reviewing this that the object structure of the reducer is slug to label string, not slug to an object of properties, and while that makes it a little less obvious, I'd still stand by my previous remarks. Interesting discussion in any case 😄

tyxla · 2016-10-11T08:01:09Z

@aduth I'd appreciate if you could take a final look before we merge this one.

aduth

I'm nitpicking at this point. It's looking really great, no blockers from me, but a few minor suggestions. Feel free to address or punt to a future PR. Otherwise good to go 👍

aduth · 2016-10-11T16:12:17Z

client/post-editor/editor-post-formats/accordion.jsx

 		} );

-		if ( isEmpty( this.props.postFormats ) ) {
+		if ( isEmpty( postFormats ) ) {


For another pull request, but noting it here, we should try to render the accordion even when data is not yet available (Reactivity and Loading States)

Ah, that's a good point. And I think this PR is the right place for that - with a clear redux state the user will never reach the post formats query component, so the post formats might never load. Fixed and thanks for noticing 👍

aduth · 2016-10-11T16:13:01Z

client/post-editor/editor-post-formats/accordion.jsx

@@ -72,10 +72,23 @@ module.exports = React.createClass( {
 				subtitle={ this.getSubtitle() }
 				icon={ <Gridicon icon="types" /> }
 				className={ classes }>
+				<QueryPostFormats siteId={ this.props.site.ID } />


Not a blocker, but we'll want to move to eliminate the site object as a prop, so it'd be better to pass the siteId from getSelectedSiteId in as a prop (even if duplicating from site for now).

Done - the post formats components will now use the siteId, which they fetch from the state. The only downside is that we still need the site object in EditorPostFormatsAccordion because siteUtils.getDefaultPostFormat uses a site object. But we can refactor that in a future PR.

aduth · 2016-10-11T16:16:23Z

client/post-editor/editor-post-formats/index.jsx

 		const { value } = this.props;

 		if ( 'standard' === value ) {
 			return 'standard';
 		}

-		const isSupportedFormat = this.getPostFormats().some( ( postFormat ) => {
-			return postFormat.slug === value;
+		const isSupportedFormat = some( this.getPostFormats(), ( postFormatLabel, postFormatSlug ) => {


If getPostFormats returns an object keyed by slug, why not express as:

const isSupportedFormat = this.getPostFormats().hasOwnProperty( value ); // Or: const isSupportedFormat = !! this.getPostFormats()[ value ]; // Or: return this.getPostFormats()[ value ] || 'standard';

Makes sense, now's a good time to simplify this logic.

aduth · 2016-10-12T12:53:08Z

client/post-editor/editor-post-formats/index.jsx


-export default React.createClass( {
+const EditorPostFormats = React.createClass( {
 	displayName: 'EditorPostFormats',


displayName becomes unnecessary when you assign React.createClass to a variable (via babel-plugin-transform-react-disdplay-name).

aduth · 2016-10-12T14:08:12Z

client/post-editor/editor-post-formats/accordion.jsx

 		} );

-		if ( isEmpty( this.props.postFormats ) ) {


So I found a subtle regression by this change. It appears that previously we were using this to account for themes which define theme support for post-formats but don't define any custom post formats. I notice for the Confit theme, for example, it has post-formats support but the /sites/%s/post-formats endpoint returns an empty object. This leads to a strange user experience where we show the accordion but there's only a single option:

In master, we'd have avoided the accordion being rendered if there was only one of these options.

So then the question becomes: Do we show the accordion by default, and only hide it after we know that either the theme doesn't support it or there are no custom post formats? Or do we hide it by default and show it only when we know there are custom post formats?

It probably depends on what's more common across themes, since it's more-or-less equally jarring for a control to appear as it is to disappear. Judgment call on your part.

But we should preserve the original behavior, which is to hide it if there are no post formats returned from the endpoint.

I've restored the previous behavior of hiding the post formats accordion if there are no post formats, but it will also call the query component in any case, so the post formats will actually be loaded.

aduth · 2016-10-12T15:00:40Z

client/post-editor/editor-post-formats/accordion.jsx

-		if ( ! this.props.post || ! this.props.postFormats ) {
-			return this.translate( 'Loading…' );
+		if ( ! post || ! postFormats ) {
+			return null;


Doesn't need to change, but FYI a child of a component can be undefined to render nothing, so this could be simplified to return; . Only in the case of the return value of render does it need to be null.

aduth · 2016-10-12T15:24:45Z

client/post-editor/editor-post-formats/accordion.jsx

+						icon={ <Gridicon icon="types" /> }
+						className={ classes }>
+						<PostFormats
+							post={ post }


Doesn't appear we use this prop in <PostFormats /> ?

Curious. We weren't using it with the previous Flux implementation as well. Omitting that.

tyxla · 2016-10-12T15:24:48Z

Looks like this needs a final review @aduth.

aduth

LGTM 👍

… accordion The Post Format Accordion does its own check for null `post` or `siteId` and doesn't render anything in that case: it waits until the `postFormats` for the site and post type are fully known. There's no need to check the `post` prop in the `EditorDrawer` component. This patch removes that check, which is one of the last remaining usages of the Flux-based post object. I also do a little cleanup of the Post Formats Accordion, namely removing the `isLoading` prop. As the component doesn't render anything at all until all data are available, it's redundant. The prop is a leftover from earlier refactoring in #8593.

tyxla added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] In Progress State labels Oct 7, 2016

tyxla self-assigned this Oct 7, 2016

tyxla force-pushed the update/post-formats-use-redux-state branch from 52f14c6 to 0749739 Compare October 10, 2016 12:47

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] In Progress labels Oct 10, 2016

mtias mentioned this pull request Oct 10, 2016

Framework: Redux single data tree and query components #5046

Closed

tyxla added this to the Offline i1: State tree consolidation milestone Oct 10, 2016

youknowriad requested changes Oct 10, 2016

View reviewed changes

youknowriad 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 10, 2016

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

youknowriad approved these changes Oct 10, 2016

View reviewed changes

youknowriad 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 10, 2016

aduth reviewed Oct 10, 2016

View reviewed changes

tyxla force-pushed the update/post-formats-use-redux-state branch from 0530461 to 079fb19 Compare October 11, 2016 07:22

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] Ready to Merge labels Oct 11, 2016

aduth approved these changes Oct 11, 2016

View reviewed changes

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

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] Ready to Merge labels Oct 12, 2016

aduth reviewed Oct 12, 2016

View reviewed changes

aduth suggested changes Oct 12, 2016

View reviewed changes

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

aduth reviewed Oct 12, 2016

View reviewed changes

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

aduth reviewed Oct 12, 2016

View reviewed changes

aduth approved these changes Oct 12, 2016

View reviewed changes

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

tyxla added 10 commits October 13, 2016 09:47

Components: Fix a typo in QueryPostFormats component

052e4a3

Editor: Use redux for post formats

26a3d17

Editor: ES6ify post formats components

01e349f

Components: Delete the PostFormatsData data component

b5dbdf0

Components: Remove the legacy PostFormatsStore

47e6b10

Editor: Decouple EditorPostFormats and EditorPostFormatsAccordion

44f2d54

Editor: Display post formats accordion while loading post formats

2fc4198

Editor: Use siteId instead of site in post formats

40c09a4

Editor: Simplify selected post format in EditorPostFormats

a9305ee

Editor: Cleanup post formats components

bbff785

tyxla force-pushed the update/post-formats-use-redux-state branch from b5de842 to bbff785 Compare October 13, 2016 06:47

tyxla merged commit 51a8e1f into master Oct 13, 2016

matticbot removed the [Status] Ready to Merge label Oct 13, 2016

tyxla deleted the update/post-formats-use-redux-state branch October 13, 2016 06:57

jsnajdr mentioned this pull request May 23, 2018

Post Editor: remove check for 'post' prop when rendering post formats accordion #25025

Merged

Editor: Use Redux state for Post Formats #8593

Editor: Use Redux state for Post Formats #8593

Conversation

tyxla commented Oct 7, 2016 • edited Loading

matticbot commented Oct 7, 2016

tyxla commented Oct 10, 2016

youknowriad left a 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

aduth Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 10, 2016 • edited 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

tyxla commented Oct 10, 2016

aduth Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

tyxla commented Oct 11, 2016

aduth left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla commented Oct 12, 2016

aduth left a comment

Choose a reason for hiding this comment

tyxla commented Oct 7, 2016 •

edited

Loading

aduth Oct 10, 2016 •

edited

Loading

aduth Oct 10, 2016 •

edited

Loading

aduth Oct 10, 2016 •

edited

Loading

aduth Oct 10, 2016 •

edited

Loading

aduth Oct 10, 2016 •

edited

Loading