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

Rich Text: Remove updateContent function #7620

Merged
merged 3 commits into from
Jul 3, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 28, 2018

This pull request seeks to simplify RichText to have a single function for setting content, vs. the current updateContent and setContent, by removing updateContent.

It's current implementation includes three notable differences from just calling setContent directly, addressed below:

Testing instructions:

Verify that there are no regressions in the behavior of the editor, particularly when splitting content, which until the merge of #7482 causes updateContent to be called.

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jun 28, 2018
@aduth aduth requested review from youknowriad and ellatrix June 28, 2018 20:31

this.savedContent = this.props.value;
this.setContent( this.savedContent );
this.editor.selection.moveToBookmark( bookmark );
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Dropping the focus management breaks the "merge". The focus is not restored at the right position when merging two paragraphs.

  • I believe the undo behavior is also necessary in this case or in cases the value of a RichText is updated programmatically (the change is not triggered by the RichText element itself)

Copy link
Member Author

Choose a reason for hiding this comment

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

But all tests have passed successfully!

😉

Copy link
Contributor

@youknowriad youknowriad Jun 29, 2018

Choose a reason for hiding this comment

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

hahaha :) we definitely need a test for that. I guess we only test the content and not the cursor position.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a failing test in dd0e347.

@aduth
Copy link
Member Author

aduth commented Jun 29, 2018

I added back the selection setting, this time with the condition that the editor has focus when content is being set. This is in combination with a new end-to-end test which would fail without the condition (as it does in master as well, even including a ZWSP in the saved content).

See 01e0501.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Looks good to me! The only thing I could find is weird undo records on split, but that's in master too.

@aduth aduth merged commit 0b4eaa8 into master Jul 3, 2018
@aduth aduth deleted the remove/rich-text-update-content branch July 3, 2018 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants