-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Make Quill instances detachable and reconfigurable #4402
base: main
Are you sure you want to change the base?
Conversation
this.emitter.removeAllListeners(); | ||
this.emitter.ignoreDOM(); | ||
this.scroll.detach(); | ||
return true; |
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.
Correct me if I'm wrong but this method does not appear to fully tear down passed in options, modules, etc. It seems to me that this method should return the quill instance to "factory settings" no?
Additionally, any dynamically changed things (like calling quill.keyboard.addBinding
) should also be cleaned up
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.
This method is not supposed to restore the quill instance to "factory settings", that would be the job of configure()
called without arguments. detach()
makes the Quill instance unresponsive to all events and user actions.
This is also why I didn't reset dynamically changed modules (keyboard, clipboard, history, etc.): a detached Quill instance is "dead", and not meant to be used anymore until reconfigured via the configure()
method
@@ -234,6 +235,7 @@ class Quill { | |||
} | |||
const scrollBlotName = Parchment.ScrollBlot.blotName; | |||
const ScrollBlot = this.options.registry.query(scrollBlotName); | |||
console.log(ScrollBlot); |
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.
🗑️ ?
@@ -27,6 +28,14 @@ class Theme { | |||
protected options: ThemeOptions, | |||
) {} | |||
|
|||
detach() { | |||
Object.values(this.modules).forEach((module) => { | |||
if (module instanceof Module) { |
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 wonder if instead a better check might be if ('detach' in module)
. You can register things with quill that aren't literally subclasses of Module
Motivation
This PR addresses discussion #4397 and issue #4234, providing a way to apply a new configuration to an existing Quill editor. Please refer to those for further information about the reasons behind this change.
Proposed changes
Subscriber
class, acting as a proxy to theEventTarget
API and keeping track of all subscriptions to event listeners originated by Quill components and modules,Subscriber
instances to Quill root elements.Subscriber
instances are stored in aWeakMap
, similar to what happens for Quill instances.addEventListener()
,detach()
,detach()
cleanup method to Quill instances,configure()
method to apply a new configuration to an already existing instance of Quill,