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.

Sign up for GitHub

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

RichText Trigger onChange on input #4955

Merged
merged 6 commits into from
Feb 12, 2018
Merged

RichText Trigger onChange on input #4955

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94c816f
WIP: try editable onChange on input
ellatrix Feb 8, 2018
fe6941f
RichText: Throttle changes for performance reasons
youknowriad Feb 9, 2018
8127ba2
RichText: Optimize performance by using strict equality instead of is…
youknowriad Feb 9, 2018
efff741
Fix issue when editor is not initialized yet
youknowriad Feb 9, 2018
6f334f0
RichText: cancel throttled functions
youknowriad Feb 12, 2018
0728800
Check editor dirtyness to avoid unnecessary onChange calls (fixes und…
youknowriad Feb 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 14 additions & 23 deletions blocks/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
find,
defer,
noop,
throttle,
} from 'lodash';
import { nodeListToReact } from 'dom-react';
import 'element-closest';
Expand Down Expand Up @@ -92,6 +93,7 @@ export class RichText extends Component {
this.getSettings = this.getSettings.bind( this );
this.onSetup = this.onSetup.bind( this );
this.onChange = this.onChange.bind( this );
this.throttledOnChange = throttle( this.onChange.bind( this ), 500 );
Copy link
Member

Choose a reason for hiding this comment

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

If we can, I'd love if we would avoid throttling. Case in point: We're not cancelling a scheduled change when the component unmounts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, if the performance fix is enough, I'm fine dropping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried dropping it but I think performance-wise it's slightly better if we keep it

Copy link
Member

Choose a reason for hiding this comment

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

It seems like it always dispatched change when I type the first character. Should we wait until 500ms passes instead?

Copy link
Member

@ellatrix ellatrix Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@youknowriad What can we do to get rid of this? :)

this.onNewBlock = this.onNewBlock.bind( this );
this.onNodeChange = this.onNodeChange.bind( this );
this.onKeyDown = this.onKeyDown.bind( this );
Expand Down Expand Up @@ -149,6 +151,7 @@ export class RichText extends Component {
editor.on( 'BeforeExecCommand', this.maybePropagateUndo );
Copy link
Member

Choose a reason for hiding this comment

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

Did we test to see how TinyMCE's undo history interacts with our own?

  • Are we calling onChange with the correct value after TinyMCE finishes undo?
  • Should we be bypassing TinyMCE's undo altogether (also preventing it from accumulating any history)?

Copy link
Member

Choose a reason for hiding this comment

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

@aduth I'm proposing these changes in #4956. Good point about preventing TinyMCE accumulating any history at all. We just need the signals, not the store.

editor.on( 'PastePreProcess', this.onPastePreProcess, true /* Add before core handlers */ );
editor.on( 'paste', this.onPaste, true /* Add before core handlers */ );
editor.on( 'input', this.throttledOnChange );

patterns.apply( this, [ editor ] );

Expand Down Expand Up @@ -366,21 +369,16 @@ export class RichText extends Component {
}
}

fireChange() {
this.savedContent = this.getContent();
this.editor.save();
this.props.onChange( this.state.empty ? [] : this.savedContent );
}

/**
* Handles any case where the content of the tinyMCE instance has changed.
*/
onChange() {
// Note that due to efficiency, speed and low cost requirements isDirty may
// not reflect reality for a brief period immediately after a change.
if ( this.editor.isDirty() ) {
this.fireChange();
if ( ! this.editor.isDirty() ) {
return;
}
this.savedContent = this.state.empty ? [] : this.getContent();
this.props.onChange( this.savedContent );
this.editor.save();
}

/**
Expand Down Expand Up @@ -508,8 +506,6 @@ export class RichText extends Component {
return;
}

this.fireChange();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?


const forward = event.keyCode === DELETE;

if ( this.props.onMerge ) {
Expand Down Expand Up @@ -698,13 +694,8 @@ export class RichText extends Component {
this.editor.save();
}

setContent( content ) {
if ( ! content ) {
content = '';
}

content = renderToString( content );
this.editor.setContent( content );
setContent( content = '' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice refactoring :)

this.editor.setContent( renderToString( content ) );
}

getContent() {
Expand All @@ -713,21 +704,20 @@ export class RichText extends Component {

componentWillUnmount() {
this.onChange();
this.throttledOnChange.cancel();
}

componentDidUpdate( prevProps ) {
// The `savedContent` var allows us to avoid updating the content right after an `onChange` call
if (
!! this.editor &&
this.props.tagName === prevProps.tagName &&
this.props.value !== prevProps.value &&
this.props.value !== this.savedContent &&
! isEqual( this.props.value, prevProps.value ) &&
! isEqual( this.props.value, this.savedContent )
this.props.value !== this.savedContent
Copy link
Member

Choose a reason for hiding this comment

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

It seems like shallow compare is enough here.

) {
this.updateContent();
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Removed line break by accident?

componentWillReceiveProps( nextProps ) {
if ( 'development' === process.env.NODE_ENV ) {
if ( ! isEqual( this.props.formatters, nextProps.formatters ) ) {
Expand All @@ -745,6 +735,7 @@ export class RichText extends Component {
this.editor.focus();
this.editor.formatter.remove( format );
}

applyFormat( format, args, node ) {
this.editor.focus();
this.editor.formatter.apply( format, args, node );
Expand Down