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

Editor: PostTextEditor: Fix case where empty state value defers to prop #9513

Merged
merged 2 commits into from Sep 3, 2018

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Aug 31, 2018

Extracted from #9403

This pull request seeks to fix a bug with PostTextEditor where intended state value is not always reflected in the rendered textarea when empty. Since previously, it relied on a simple truthy condition to determine whether to use state or props value, if the user had erased all content from the textarea, it would effectively trigger this fallback to props value to occur, thus disregarding the local (user-intended) state.

In practice, this had no effect only because props edit state was kept in sync, so the value remained the same. The issue became more obvious through related refactoring in #9403.

Testing instructions:

There should be no regressions in the behavior of the PostTextEditor.

Ensure unit tests pass:

npm run test-unit editor/src/components/post-text-editor/test/index.js

Future tasks:

In testing this myself, I discovered some unfortunate consequences of our "delayed sync" blocks parsing behavior. Since we rely on the blur event to persist content, the sync doesn't always occur, for example when using the keyboard shortcut to switch between modes. I created an issue at #9512 to track (with potential fix included).

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 31, 2018

Member

The new approach in 3b2444e is much simpler, and leverages isDirty as the key indicator for whether to derive from props.

Member

aduth commented Aug 31, 2018

The new approach in 3b2444e is much simpler, and leverages isDirty as the key indicator for whether to derive from props.

* @see stopEditing
*
* @param {Event} event Change event.
*/
edit( event ) {
const value = event.target.value;
this.props.onChange( value );

This comment has been minimized.

@youknowriad

youknowriad Sep 3, 2018

Contributor

So this is a bit weird :). I suppose this is only needed to trigger the post dirtiness, right? I wonder if a specific action is not better.

@youknowriad

youknowriad Sep 3, 2018

Contributor

So this is a bit weird :). I suppose this is only needed to trigger the post dirtiness, right? I wonder if a specific action is not better.

This comment has been minimized.

@youknowriad

youknowriad Sep 3, 2018

Contributor

Granted it's not being changed in this PR

@youknowriad

youknowriad Sep 3, 2018

Contributor

Granted it's not being changed in this PR

This comment has been minimized.

@aduth

aduth Sep 6, 2018

Member

Can you elaborate on what's weird here? onChange is called to surface the edit to the editor store immediately (so as to mark for dirty detection).

@aduth

aduth Sep 6, 2018

Member

Can you elaborate on what's weird here? onChange is called to surface the edit to the editor store immediately (so as to mark for dirty detection).

This comment has been minimized.

@youknowriad

youknowriad Sep 6, 2018

Contributor

Why are we filling the content in the edits? For me the blocks is the edits of the content property. And we're already filling the blocks properly when persisting. I bet this is only needed to trigger a "dirty" state.

@youknowriad

youknowriad Sep 6, 2018

Contributor

Why are we filling the content in the edits? For me the blocks is the edits of the content property. And we're already filling the blocks properly when persisting. I bet this is only needed to trigger a "dirty" state.

This comment has been minimized.

@aduth

aduth Sep 6, 2018

Member

I bet this is only needed to trigger a "dirty" state.

Pretty much this, yes 😄

@aduth

aduth Sep 6, 2018

Member

I bet this is only needed to trigger a "dirty" state.

Pretty much this, yes 😄

This comment has been minimized.

@youknowriad

youknowriad Sep 6, 2018

Contributor

Is this better than a SET_DIRTY action or something like that? It's a bit clearer

@youknowriad

youknowriad Sep 6, 2018

Contributor

Is this better than a SET_DIRTY action or something like that? It's a bit clearer

This comment has been minimized.

@aduth

aduth Sep 6, 2018

Member

It's an option, one which would contradict the purpose of proposed #7409.

@aduth

aduth Sep 6, 2018

Member

It's an option, one which would contradict the purpose of proposed #7409.

This comment has been minimized.

@aduth

aduth Sep 6, 2018

Member

I don't necessarily agree that SET_DIRTY is clearer in the broader sense. In practical terms as it impacts what we're doing here, sure, the intent with calling editPost is to make sure the post becomes marked as dirty, but dirtiness is not really meant to be an explicit action, more a reflection of the current state (diff of edits and saved post). It's by various complexities of content/blocks syncing that we've landed at our current implementation, but I'd rather not further engrain ourselves to this idea of "make dirty" if we can help it.

@aduth

aduth Sep 6, 2018

Member

I don't necessarily agree that SET_DIRTY is clearer in the broader sense. In practical terms as it impacts what we're doing here, sure, the intent with calling editPost is to make sure the post becomes marked as dirty, but dirtiness is not really meant to be an explicit action, more a reflection of the current state (diff of edits and saved post). It's by various complexities of content/blocks syncing that we've landed at our current implementation, but I'd rather not further engrain ourselves to this idea of "make dirty" if we can help it.

@youknowriad

LGTM 👍

@noisysocks noisysocks merged commit 71c4d8f into master Sep 3, 2018

2 checks passed

codecov/project 50.37% (+0.14%) compared to 6a5ac6a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the fix/post-text-editor-local-state branch Sep 3, 2018

@mtias mtias added this to the 3.8 milestone Sep 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment