Skip to content
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

Merged
merged 3 commits into from Sep 1, 2022
Merged

Change read only mode API to editable mode #2912

merged 3 commits into from Sep 1, 2022

Conversation

trueadm
Copy link
Collaborator

@trueadm trueadm commented Aug 30, 2022

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()
  • editor config { readOnly: true } -> { editable: boolean }

@vercel
Copy link

vercel bot commented Aug 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Sep 1, 2022 at 4:15AM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Sep 1, 2022 at 4:15AM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 30, 2022
@trueadm trueadm changed the title Remove read only mode API, make dit mode Change read only mode API to editable mode Aug 30, 2022
@trueadm trueadm marked this pull request as ready for review August 30, 2022 14:45
@zurfyx
Copy link
Member

zurfyx commented Aug 30, 2022

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?

@thegreatercurve
Copy link
Contributor

@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.

@acywatson
Copy link
Contributor

acywatson commented Aug 31, 2022

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.

@amyworrall
Copy link
Contributor

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:

  1. Can users edit the text by selecting and typing
  2. Can developers edit the contents programatically at all
  3. Should developers be editing the contents programatically right now?

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 update{} block or not. If, for example, we're in a read{} block, developers can't update the editor state right now. Or at least, they shouldn't.

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 editable. Number 3 should be called readOnly. And number 2 shouldn't have a name because it's not needed.

On iOS, UITextView has an isEditable property. If that is false, then keystrokes, pasting text, etc are rejected. But there's nothing stopping the devs from going in to the NSTextStorage and changing the text directly. So there is precedent for these semantics on at least one platform.

@trueadm
Copy link
Collaborator Author

trueadm commented Aug 31, 2022

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.

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.

@acywatson
Copy link
Contributor

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!

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.

@acywatson acywatson merged commit 06c3e61 into main Sep 1, 2022
@trueadm trueadm deleted the edit-mode branch September 1, 2022 22:36
@acywatson acywatson mentioned this pull request Sep 3, 2022
thegreatercurve pushed a commit that referenced this pull request Nov 25, 2022
* Remove read only mode API, make dit mode

* Fix docs

* Fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants