search

angular-ui / ui-tinymce

AngularUI wrapper for TinyMCE
MIT License
488 stars 371 forks source link

Fix/issue 224 performance improvement #226

Closed abtincbrians closed 8 years ago

abtincbrians commented 8 years ago

This is an update to address poor performance when there's a lot of text in the WYSIWYG editor, as discussed in https://github.com/angular-ui/ui-tinymce/issues/224.

This debounces the save and view update methods, improving UI responsiveness tremendously when typing while there's a large amount of text in the text area.

deeg commented 8 years ago

Would you mind posting a plunker with the fix so I can see the changes?

Also, I am trying to get people in the habit of adding tests when they submit PRs. If you have the time please add some tests, but if you don't I will still consider merging since this fixes a performance issue, and our tests are not up to date right now.

Thanks for putting out this PR, I will take a closer look tonight.

deeg commented 8 years ago

I also think the reason this may be happening is some of those events which are triggered overlap and the update is getting called more than once when it isn't needed.

I want to do a bit of investigation before merging this in.

abtincbrians commented 8 years ago

New Plunker here: https://plnkr.co/edit/zv3Gb3y4PLsjXvV5HEaa The original version is here: https://plnkr.co/edit/mpRJHBr67962YqWdJ4UF

The performance is still impaired compared to TinyMCE (sans Angular & ui-tinymce), however, the UI responsiveness is considerably improved with this code update.

I would recommend debouncing the save & update routine even if you are able to identify and resolve the overlapping events. The change event is fired on every character that is typed, causing the ed.save() function to run in rapid succession. This function is resource intensive. Debouncing eliminates excessive calls to the ed.save() method -- fewer (unnecessary) calls means better performance.

I haven't reviewed the tests, other than to run them before submitting the pull request to make sure none of them failed. Time permitting I'll review further to see if I have something to contribute.

@deeg Tagging you, hopefully you'll see this.

deeg commented 8 years ago

Sorry for the delay. I will try to get to this tonight.

deeg commented 8 years ago

@abtincbrians, again sorry this took a little while.

I agree with you we should debounce regardless, so I merged in your change. Thanks for the PR!