solid-codemirror icon indicating copy to clipboard operation
solid-codemirror copied to clipboard

createEditorReadonly sets EditorView.editable.of(false)

Open sparecycles opened this issue 1 year ago • 2 comments

First of all, thanks for this library!

I just had a small hiccup using createEditorReadonly along with

import { search, searchKeymap } from "@codemirror/search";

createExtension(() => [search(), keymap.of(searchKeymap)]);

Where the search would not show up in my "read-only" views.

The docs for state.EditorState^readOnly call out

Not to be confused with EditorView.editable, which controls whether the editor's DOM is set to be editable (and thus focusable).

So, for me, the fix was to replace my use of createEditorReadonly with codemirror's EditorState.readOnly which allows focus and allows command-F to bring up the search panel.

createExtension(() => props.readonly ? EditorState.readOnly.of(true) : undefined);

I'm kind of thinking that createEditorReadonly should be deprecated and users should be encouraged to do the following instead?

createExtension(() => props.readonly ? EditorState.editable.of(false) : undefined);

( Unless there's some edge case in the behavior I'm not aware of that createEditorReadonly is solving. )

TL;DR

  • createEditorReadonly sets editable=false, which is confusing/mislabled because "read only" and "not editable" are two different concepts for codemirror.
  • createEditorReadonly seems to be an unnecessary entrypoint into the library, replaceable with a note in the documentation.

sparecycles avatar Aug 13 '24 19:08 sparecycles

Hi, sorry for the late response but I was off.

Yes, the name of the function is wrong as you noticed. I made the extensions folder to group most behaviors that can be integrated easily with signals. In theory both editable and readOnly are unnecessary entrypoints as you said.

Would you like to open a pr for this?

riccardoperra avatar Aug 27 '24 18:08 riccardoperra

Just saw your response. I wouldn't mind making a PR but I'm mostly glad you've confirmed that there isn't some edge case (that we're aware of, anyway).

sparecycles avatar Sep 06 '24 19:09 sparecycles