search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Added "shouldUndo" to scribe options for optional undo management #344

Closed cutandpastey closed 9 years ago

cutandpastey commented 9 years ago

As mentioned here: https://github.com/guardian/scribe-plugin-advanced-undo/pull/1 I've tried to make scribe undo functionality optional by configuration. Not 100% sure of all the impacts of this so would be good to get eyes @rrees @hmgibson23 @robinedman @jamespamplin @theefer

NB: all the tests run locally.

theefer commented 9 years ago

Sounds like we want to identify the behaviour that is achieved by interacting with the undo manager in these patches (writing test for these cases if possible) before we make it optional?

cutandpastey commented 9 years ago

I don't feel that is necessary. The only two places it's not totally encapsulated is in src/plugins/core/events.js & src/plugins/core/patches/events.js where the call to undo is there to clear the last undo item before formatters run. If undo is optional then this clearly isn't required.

hmgibson23 commented 9 years ago

I'm fine with this :+1:

theefer commented 9 years ago

OK so no need to cleanup the UndoManager's history because the UndoManager is completely bypassed? Fair enough.

Seems like we'll want a cleaner way to factor out the current undo manager and replace it with another one, in the future.

theefer commented 9 years ago

:+1:

cutandpastey commented 9 years ago

@rrees made all requested amends.

rrees commented 9 years ago

:+1: