-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RichText Trigger onChange on input #4955
Conversation
See: #2758 (Particularly bits about undo history) |
18e97c2
to
571d61d
Compare
I'm currently rebasing a WIP branch that does the undo signalling with a buffer in the history. I'm also leaving out selection now since it would make it more complex. I feel like we should fix undo together this PR. |
blocks/rich-text/index.js
Outdated
|
||
// Saving the editor on updates avoid unecessary onChanges calls | ||
// These calls can make the focus jump | ||
this.editor.save(); | ||
} | ||
|
||
setContent( content ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add content = ''
here 🙈
e6af488
to
87ca673
Compare
So from my testing, performance is good enough here but I'd appreciate that other people try it to confirm Edit: Erratum: On long post it's not good at all, needs further investigation |
@@ -92,6 +93,7 @@ export default 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 ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? :)
@youknowriad I created a branch off of this one which includes an iteration on https://github.com/WordPress/gutenberg/tree/try/weak-map-cache See: aduth/rememo#1 |
@aduth Awesome, I'm not seeing any performance issues any more 👍 On the performance side, maybe instead of using |
15fa35c
to
efff741
Compare
Rebase this with the changes from #5002 it seems better now. |
@youknowriad, see https://github.com/WordPress/gutenberg/pull/5002/files#r167532619, can we bring the polyfill as part of this PR? |
|
||
content = renderToString( content ); | ||
this.editor.setContent( content ); | ||
setContent( content = '' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring :)
blocks/rich-text/index.js
Outdated
if ( this.editor.isDirty() ) { | ||
this.fireChange(); | ||
} | ||
this.savedContent = this.state.empty ? [] : this.getContent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth - should we extract []
to its own shared variable to ensure we have always one variable reference to compare with ===
? I wanted to double check - you know better all those perf bits. It might not make any sense in this context :)
this.props.value !== this.savedContent && | ||
! isEqual( this.props.value, prevProps.value ) && | ||
! isEqual( this.props.value, this.savedContent ) | ||
this.props.value !== this.savedContent |
There was a problem hiding this comment.
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.
|
I'd be inclined to omitting the I'll try to take a pass at these today (non-blocking). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested in enough. Let's merge it and continue in the follow-up PRs as discussed 👍
) { | ||
this.updateContent(); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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?
@@ -508,8 +506,6 @@ export class RichText extends Component { | |||
return; | |||
} | |||
|
|||
this.fireChange(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
I don't know, this was part of one of your commits, I can restore :) |
@youknowriad I think I removed that there assuming changes would be in sync all the time, but with the throttle that wouldn't be the case? |
Noting that in the course of writing a post with a single paragraph containing only 32 words, I accumulated 71 copies of editor state via undo history (and that this will only grow over the course of writing a post in the same session). |
Something revealed by this change: Should we be debouncing autosave instead of throttling? It feels as though it saves much more frequently now, since it can occur many times while typing within the same paragraph block. Debouncing isn't great either, since just because I don't stop typing for X amount of time doesn't mean the post should never save during that time. Or maybe the autosave should just be made less frequent than every 10 seconds? |
@@ -149,6 +151,7 @@ export class RichText extends Component { | |||
editor.on( 'BeforeExecCommand', this.maybePropagateUndo ); |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth I don't mind if we save frequently, but it is quite visually distracting ATM. Sounds like it need to be more subtle, similar to Google Docs. We also have to make sure we don't just pile these changes up as revisions, instead we should maybe reuse the last revision and only create a new revision on a user action or at least way less often (each revision is a post in the DB). |
Yeah, this is an ongoing issue unfortunately: #3656 |
This will be necessary to achieve #4951
It also fixes some issues while the save draft link doesn't show up until we focus out of the component.
It may introduce a difference in a way undo/redo works but I believe we should handle those consistently (it's not the case right now, RichText triggers and undo state per word while PlainText it's per character). I'm thinking we could use the
withHistory
a Reducer enhancer to group similar changes into one undo level.Testing instructions