-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Change read only mode API to editable mode #2912
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
LGTM, @acywatson I wonder if you have thoughts on this since you were exploring the possibility to block updates by default when the read-only flag is true? |
@zurfyx Should we release this as part of a 0.4 release as it contains breaking changes? There are a few other 0.4.0 TODOs we need to switch over as well. |
Thanks for flagging @zurfyx - I missed this. I don’t think this makes anything much clearer, to be honest. Just inverting the semantics seems like churn. You can still edit when editable is false, so I’m not sure this is better. My plan is to actually start enforcing read only in the core, which would make this make even less sense. |
I think there's three related concepts here:
In my opinion: Number 2 will almost always be true. I can't see a reason to put Lexical in a mode where developers can't update the editor state at all. If developers don't want the editor state to be updated, they should just not update it! Number 3 relates to whether we're inside an Number 1 is purely a user interaction setting. If it's turned off, we don't handle keystrokes that mutate the editor state. This is useful regardless of whether we allow developers to make their own changes to the editor state. IMO. number 1 should be called On iOS, |
I think it's clearer that editing relates to a user interactions, vs read-only which relates to a CS relates concept around editing memory and also clashes with our other read-only naming for editor states. This is also what ProseMirror uses https://prosemirror.net/docs/ref/#view.EditorProps.editable, and also relates closer to the DOM property and to other attributes in other frameworks. You're right in that it's an inverse. So if they team doesn't feel like introducing the churn of renaming, we can close this PR. |
I follow you, but as a practical matter this means picking through a web of command listeners and nested update calls and conditioning each one on the current "editable" state of the editor. The idea would be to prevent mutations by default without the need for such a condition everywhere, but allow for updating the editor in this mode with an override flag. This is more of a DX thing anything else. This has actually been pretty difficult to implement given the way command listeners are tightly coupled with editor updates (I think this could cause more issues down the road, since we expose options on update that allow the user to change how the update is applied and those can't be passed to the internal update that wraps command listeners in the core). So I'm almost at the point where I think we need to just wrap everything in an "editable" check and call it a day anyway. |
* Remove read only mode API, make dit mode * Fix docs * Fixes
Read only mode is confusing due to its wording. You can still modify the editor in read only mode, so is it really read only? Surely, the mode should be is editable to reflect the fact that the underlying DOM element is content editable.
The following APIs have changed:
editor.isReadyOnly
->editor.isEditable()
editor.setReadyOnly
->editor.setEditable()
editor.registerReadOnlyListener
->editor.registerEditableListener()
{ readOnly: true }
->{ editable: boolean }