Editor: Reduxify The EditorTitle component #8814

youknowriad · 2016-10-18T14:44:13Z

Drop sitesList usage Framework: remove sitesList.getSelectedSite() usages #8726
Drop PostEditStore calls
Rewrite using the last coding style (ES6 class)

Testing instructions

Create a new post and edit its title
The title should update correctly
Do the same thing for different post types

cc @aduth

matticbot · 2016-10-18T14:44:15Z

Test live: https://calypso.live/?branch=update/reduxify-editor-title

dmsnell · 2016-10-18T17:16:14Z

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

+			isNew,
+			post,
+			site,
+			ownProps


why are we adding ownProps here? it will come in as this.props.ownProps - is that the intention? also, did you know that ownProps automatically gets merged into the props through connect even if we never use them?

Yes I know, the idea is that ownProps needs to be passed down the textarea. The previous way of doing this was to rely on ownProps automatically merged to the props and using omit to exclude the current component props.

And when using localize and connect etc... we get extra props that we don't want to pass down to the textarea (we'll get warnings). And it's hard to keep those ignored props up to date with this method. So, for now, localize adds three props (translate, moment and another one). Imagine someone adding another prop to localize, he won't think of fixing these kind of issues (adding the new Prop to omit). That's why I used this technique.

I could also replace the ...ownProps with explicit props passing (we use only tabIndex for now I think), but I see the component as an upgraded textarea, so It's "logic" to just pass all parent props down.

I'm sorry @youknowriad but I don't think I understand what you are saying here. what it appears like you are saying doesn't fit with what I think the code is saying.

what is the problem with localize() and why does it break things? also, one of my chief questions here is that we are forcing the component/children to change their logic here because if they formerly depended on this.props.value they will now have to call out to this.props.ownProps.value

Yes, you are right! my answer was not so clear. Let me explain more. the PostEditor component renders the EditorTitle Component like this : <EditorTitle tabIndex="1" /> and the EditorTitle Component renders a textarea close to this : <Textarea {...props />.

The issue here, is to find the best way to compute the props that needs to be provided to the Textarea. In the example here, these props are { tabIndex: '1' }. We need to find the best way to find these exact props without passing any extra prop provided by the HoC components used on the EditorTitle.

The options are :
1- const props = omit( this.props, Object.keys( this.constructor.propTypes ) ) and make sure we define all the propTypes provided by HoC like localize even If we don't use them like the moment prop. Also If the localize HoC get updated and provides an extra prop, the EditorTitle component will have to be updated to "omit" the new prop.

2- using ownProps since we know these are the only props provided to the EditorTitle container and not computed by any HoC.

3- avoid using { ...props } and use only something like tabIndex={this.props.tabIndex}. This approach is fine but we change the behavior of the EditorTitle component since we can not provide extra props (for example style={someStyle} or whatever) to the textarea

I hope this is clearer.

I think I understand what you are saying now @youknowriad. it seems like an indication that bigger issues exist here.

we have plenty of places where we pass ...props down through several levels of nesting and that is almost always really confusing. I'm a huge advocate for explicit prop passing. props passed by localize shouldn't be passed along the chain either because if a child needs translate then it should be directly wrapped by localize

concerning passing other props I wonder if it's not more of a concern than it seems. why should we want to pass some new style to an underlying textarea inside a component? that actually breaks our React idioms, that parents should need to know nothing of the inner workings of their children. perhaps being explicit about a component's public API and keeping the nested children isolated from the grandparents can provide more robustness and reliability here.

I'm ok with using explicit props. I think it's a good solution. My concern about it was that It changes the way this component works (and it was not the purpose of this PR, just a refactor).

From my understanding, this component was considered like a "decorator" of the textarea and this won't be the case anymore with explicit props but I'm ok with it.

I'll update the PR to reflect that :)

we shouldn't be changing the behavior, but wouldn't this.props.ownProps also change it?

No, it does not change it. If we compare the props passed down to the textarea before the refacto and after (with ownProps), they'll remain exactly the same.

youknowriad · 2016-10-19T10:17:43Z

@dmsnell I update the RP with explicit props. It's probably better. We'll just pass down new props when needed and avoid the hassle of figuring out the right props to pass down.

youknowriad · 2016-11-04T08:42:39Z

Any other thoughts about this @dmsnell tks :)

dmsnell · 2016-11-04T12:59:53Z

Sorry for missing this @youknowriad - I think it's probably ready to go. I haven't tested it so that's the catch. Please merge at your leisure or what on someone else to review if you prefer. Again, sorry - I'm a little swamped at the moment.

youknowriad · 2016-11-04T14:01:40Z

Don't worry @dmsnell I totally understand, we can all be overwhelmed :)

I'll test it deeply again and merge this up. Tks

aduth · 2016-11-04T15:20:25Z

@youknowriad Does this account for empty content checking that exists in the Flux implementation? Will explain shortly in follow-up, just wanted to catch before merge.

aduth · 2016-11-04T15:25:12Z

Specifically, see #5445 which is a similar refactoring that accounts for post having content. A post cannot be saved unless it has one of these fields not being empty: title, content, excerpt. The editor Publish and Save buttons should be disabled until one of these conditions is met. That is currently managed via PostEditStore.hasContent. If we were to update this component to set title in Redux state, we would need for this state to be considered too in whether Publish is active.

aduth · 2016-11-04T15:25:50Z

Steps to reproduce:

Start new post
Enter title

Expected: I can publish
Actual: I can't publish

youknowriad · 2016-11-07T10:52:23Z

Oh! I missed that, thanks @aduth

I updated the PR by using the same behavior we had for the post dirty checking. Basically, to check if the post has content, we use the PostEditStore as well as a new redux selector editedPostHasContent. And after moving all the Editor's parts to Redux, we'll just stick with the selector.

Thoughts?

aduth · 2016-11-07T16:25:32Z

client/state/posts/selectors.js

+  * @param  {Number}  postId Post ID
+  * @return {Boolean}        Whether the edited post has content or not
+ */
+export const editedPostHasContent = createSelector(


Could this selector be simplified to:

const editedPost = getEditedPost( siteId, postId ); return ( some( [ 'title', 'excerpt' ], ( field ) => !! editedPost[ field ].trim() ) || ! isPostContentEmpty( editedPost.content ) );

Notably leveraging getEditedPost instead of recreating that behavior.

Tks @aduth I was not aware getEditedPost was handling the merge thing 👍

dmsnell · 2016-11-08T10:44:36Z

If this is a memorized selector, as via createSelector(), but the return value depends on a property of something not in the parameter list, won't the memorization lead to bugs?

Post content changes but code is requesting the same site/post ids, memorized selector thusly returns previous value of emptyPost even though it's no longer empty…

youknowriad · 2016-11-08T10:57:27Z

If I understand correctly the behavior of createSelector there's no bug here, since the cache will be used only if we have the same sub-state (computed from the declared dependencies : the second parameter of createSelector) and also the exact same arguments.

https://github.com/Automattic/wp-calypso/tree/fcf789ebb43b7d8fe9a912d8fa82d5bee66573b5/client/lib/create-selector

youknowriad · 2016-11-08T11:00:10Z

At least, if it's not the case, then we should update the example on the README there

dmsnell · 2016-11-08T11:47:19Z

there's no bug here, since the cache will be used only if we have the same sub-state

is it the case then that we are "flushing" the memorized cache any time any post changes or is added/removed? is the memorized selector even helping us that much?

did we have performance issues with this title that led us to forego the simplicity of a straightforward selector?

youknowriad · 2016-11-08T11:53:36Z

@dmsnell to be honest, I have not experienced specific performance any issue but I used this approach to mimic what has been done for isEditedPostDirty selector which is used in the exact same way as this selector.

So yes, I'm ok to drop the memoization unless we experience performance issues.
Any thought on this @aduth ?

aduth · 2016-11-08T15:29:13Z

is it the case then that we are "flushing" the memorized cache any time any post changes or is added/removed?

Yes, this is true. The members of the second argument of createSelector are compared between calls and if any is not strictly equal to its previous value, the cache is cleared.

is the memorized selector even helping us that much?

The most benefit comes from those selectors used often and perform heavy logic. Since connect selectors are run very frequently, it can save hundreds or thousands of function invocations (multiplied by number of multiple components in the same screen), especially if we don't expect the state to change very often.

Arguments from the "de facto" solution reselect: https://github.com/reactjs/reselect#motivation-for-memoized-selectors

The hope was for createSelector to be unobtrusive (simple API). With a memory overhead of storing the cache, it's not always appropriate, though I feel it's a nice optimization. In this case, I could see it going either way. I personally don't think it's an inappropriate use of createSelector.

dmsnell · 2016-11-08T15:42:05Z

it can save hundreds or thousands of function invocations

from the code of the memorized selector @aduth it looked like it might not save us any function calls. it looked like this one was simply checking if a string is empty while the memorized selector also makes several function calls

aduth · 2016-11-08T15:47:38Z

simply checking if a string is empty

There's more going on in the selector than just that, but sure, there's a balance to be had between the logic of the memoize function and the selector itself.

aduth · 2016-12-07T15:18:29Z

client/state/sites/selectors.js

+ * @return {Boolean} true if the site's permalinks are editable
+ */
+export function areSitePermalinksEditable( state, siteId ) {
+	const site = getRawSite( state, siteId );


Would be good to leverage getSiteOption here I think.

youknowriad · 2016-12-07T15:51:52Z

Rebased and fixed the last notes :)

aduth

Observed strange behavior that may not be specifically related to these changes but more apparent.

Start new post
Enter title
Before post saves, open drafts drawer and select another post
Accept warning that changes will be lost
Observe draft loads correctly
Start new post

Expected: Fresh post
Actual: Title from step 2 is included

aduth · 2016-12-08T15:16:22Z

client/state/posts/selectors.js

+  * @param  {Number}  postId Post ID
+  * @return {Boolean}        Whether the edited post has content or not
+ */
+export function editedPostHasContent( state, siteId, postId ) {


Ideally the has (or is, or get) would come first in the selector name as an indicator of the type of value it returns... but I'm struggling to come up with a better name 😄

I tend to agree, but here it's hard to follow this rule and have something meaningful. Naming is hard 😅

I support putting has in the middle. reads much easier for A has B functions

youknowriad · 2016-12-20T12:58:48Z

@aduth The strange behaviour regarding the remaining title edit while switching post is not directly related to this PR. Fix is here #10177

* Drop sitesList usage * Drop PostEditStore calls

…ost check * This allows us to have the same behavior as dirty checking while still moving to redux. Basically we check for content presence on Redux and on EditPostStore.

youknowriad · 2017-01-03T15:11:51Z

@aduth I rebase this, and since #10177 has been merged, we may be good to go here?

aduth

Looked over code and took it for another spin, all appears to work well 👍

I'd much rather the new selectors (and maybe even updated selectors) be moved to state/selectors, but not opposed to merging as-is.

aduth · 2017-01-04T16:06:56Z

client/state/posts/selectors.js

+	return (
+		!! editedPost &&
+		(
+			some( [ 'title', 'excerpt' ], ( field ) => editedPost[ field ] && !! editedPost[ field ].trim() ) ||


No need to change, but I like how Lodash methods handle the falsey case for you gracefully. In this case it could be simplified to:

some( [ 'title', 'expect' ], ( field ) => trim( editedPost[ field ] ) )

aduth · 2017-01-04T16:10:54Z

client/state/posts/selectors.js

+		!! editedPost &&
+		(
+			some( [ 'title', 'excerpt' ], ( field ) => editedPost[ field ] && !! editedPost[ field ].trim() ) ||
+			/*


Nitpick: Per guidelines, multi-line comments which are not docblocks should use // (reference)

aduth · 2017-01-04T16:13:37Z

client/state/posts/utils.js

+ * @param  {string}  content Raw post content
+ * @return {Boolean}         Whether it's considered empty
+ */
+export function isEmptyContent( content ) {


Curious how we might handle these types of utility functions once selectors are all moved to the state/selectors directory. I personally don't really like them all that much, but seem an unfortunate necessity in some cases. Here, since isEmptyContent is probably only needed by the editedPostHasContent selector, I might imagine it could live alongside the selector in its state/selectors/edited-post-has-content.js file (exported only for purposes of testing, but not to be used elsewhere).

aduth · 2017-01-04T16:17:41Z

client/state/sites/selectors.js

+		return false;
+	}
+
+	return /\/\%postname\%\/?/.test( permalinkStructure );


Guessing this is partly legacy, but I don't think we need to escape % in the RegExp.

youknowriad · 2017-01-05T07:47:40Z

@aduth Valuable notes above, I'm moving editedPostHasContent and areSitePermalinksEditable to the global selectors directory before merging.

youknowriad · 2017-01-05T08:21:44Z

client/state/selectors/test/index.js

-			expect( require( '../' + kebabCase( key ) ) ).to.equal( selector );
+			const module = require( '../' + kebabCase( key ) );
+			const defaultExport = module.default ? module.default : module;
+			expect( defaultExport ).to.equal( selector );


@aduth I had to update this test to allow global selectors to export "utils" functions for test purpose.

@aduth I had to update this test to allow global selectors to export "utils" functions for test purpose.

Makes sense 👍 If/when we drop babel-plugin-add-module-exports (npm) this can be simplified to:

expect( require( '../' + kebabCase( key ) ).default ).to.equal( selector );

aduth · 2017-01-06T03:57:35Z

Still tests well for me after latest changes 👍

youknowriad added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Oct 18, 2016

youknowriad self-assigned this Oct 18, 2016

gwwar mentioned this pull request Oct 18, 2016

Framework: remove sitesList.getSelectedSite() usages #8726

Closed

76 tasks

dmsnell reviewed Oct 18, 2016

View reviewed changes

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

aduth added [Status] Needs Author Reply and removed [Status] Ready to Merge labels Nov 4, 2016

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

aduth reviewed Nov 7, 2016

View reviewed changes

aduth reviewed Dec 7, 2016

View reviewed changes

youknowriad force-pushed the update/reduxify-editor-title branch from ff8b073 to b90fe74 Compare December 7, 2016 15:51

aduth suggested changes Dec 8, 2016

View reviewed changes

aduth reviewed Dec 8, 2016

View reviewed changes

youknowriad mentioned this pull request Dec 20, 2016

Editor: Reset Post Edits when switching posts in the post editor #10177

Merged

youknowriad added 9 commits January 3, 2017 15:52

Editor: Reduxify The EditorTitle component

5bdfbde

* Drop sitesList usage * Drop PostEditStore calls

Changes per review : Explicit props passed to the textarea

db07188

Adding editedPostHasContent selector to rely on redux for the empty p…

14f1850

…ost check * This allows us to have the same behavior as dirty checking while still moving to redux. Basically we check for content presence on Redux and on EditPostStore.

Changes per review

a1ca979

Changes per review

fed1c08

memoize getEditedPost insteand of editedPostHasContent

41faa41

fix unit tests

6cc79f0

Changes per review

15e76f3

More changes per review

dd97399

youknowriad force-pushed the update/reduxify-editor-title branch from b90fe74 to dd97399 Compare January 3, 2017 15:05

aduth approved these changes Jan 4, 2017

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 Jan 4, 2017

Move the selectors to the global directory

90bea6c

youknowriad commented Jan 5, 2017

View reviewed changes

youknowriad merged commit 8e977e9 into master Jan 6, 2017

youknowriad deleted the update/reduxify-editor-title branch January 6, 2017 08:50

lancewillett removed the [Status] Ready to Merge label Jan 18, 2017

aduth mentioned this pull request Mar 1, 2017

Editor: Manage post title edits using global Redux state #5445

Closed

mtias mentioned this pull request Mar 1, 2017

Editor: Autosave reverts content typed while autosaving #11567

Closed

Editor: Reduxify The EditorTitle component #8814

Editor: Reduxify The EditorTitle component #8814

Conversation

youknowriad commented Oct 18, 2016 • edited by artpi Loading

matticbot commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Oct 18, 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

Choose a reason for hiding this comment

youknowriad commented Oct 19, 2016

youknowriad commented Nov 4, 2016

dmsnell commented Nov 4, 2016

youknowriad commented Nov 4, 2016

aduth commented Nov 4, 2016

aduth commented Nov 4, 2016

aduth commented Nov 4, 2016

youknowriad commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Nov 8, 2016

youknowriad commented Nov 8, 2016

youknowriad commented Nov 8, 2016 • edited Loading

dmsnell commented Nov 8, 2016

youknowriad commented Nov 8, 2016

aduth commented Nov 8, 2016

dmsnell commented Nov 8, 2016

aduth commented Nov 8, 2016

Choose a reason for hiding this comment

youknowriad commented Dec 7, 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

youknowriad commented Dec 20, 2016

youknowriad commented Jan 3, 2017

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

youknowriad commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

aduth Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

aduth commented Jan 6, 2017

youknowriad commented Oct 18, 2016 •

edited by artpi

Loading

youknowriad Oct 18, 2016 •

edited

Loading

youknowriad commented Nov 8, 2016 •

edited

Loading

youknowriad commented Jan 5, 2017 •

edited

Loading

aduth Jan 6, 2017 •

edited

Loading