WIP: Add readonly, min-height attributes and faust-editor-basic "passive" variant #2
Conversation
as faust-web-component-basic.js, hugely reducing size and load time
|
Thanks, hoping @ijc8 can give a more expert point of view than me on the PR. |
There was a problem hiding this comment.
Hey, thanks for the PR!
In addition to the specific comments I've left, here are some general remarks:
readonlyseems good to offer (and matches the builtin<input>and<textarea>elements).- The basic editor (with a lightweight build that omits libfaust) is likewise a nice idea.
- Ditto for the copy button.
- I see the motivation for
minHeight, but I wonder we could enable the user to set this directly via their own CSS (rather than as a new, special-purpose attribute).
src/editor.ts
Outdated
| @@ -63,6 +63,7 @@ export function createEditor(parent: HTMLElement, doc: string) { | |||
| ...lintKeymap | |||
| ]), | |||
| faustLanguage, | |||
| EditorView.editable.of(editable) | |||
There was a problem hiding this comment.
I think you want the readOnly facet instead of (or perhaps in addition to) the editable facet.
editable prevents the editor from getting focus but doesn't actually prevent the contents from being modified, so it's currently possible to modify the contents of a readonly editor via e.g. cut & paste shortcuts.
src/faust-editor-basic.ts
Outdated
| if (navigator.clipboard) { | ||
| navigator.clipboard.writeText(editor.state.doc.toString()) | ||
| } else { | ||
| console.log("Unable to use clipboard") |
There was a problem hiding this comment.
Maybe we could check navigator.clipboard at template-generation time to avoid creating the copy button in the first place.
| return { | ||
| build: { | ||
| lib: { | ||
| entry: resolve(__dirname, mode == "basic" ? "src/main-basic.ts" : "src/main.ts"), |
There was a problem hiding this comment.
Sorry to hear it's been a hassle to get multiple builds working with Vite.
This mode workaround seems fine to me. Alternatively, we could maybe define two different vite configs and switch between them with -c.
There was a problem hiding this comment.
Thanks, let's keep the mode workaround for now.
src/faust-editor.ts
Outdated
| } | ||
|
|
||
| attributeChangedCallback(name, oldValue, newValue) { | ||
| if ((name == "readonly") && (newValue != null)) { |
There was a problem hiding this comment.
nit: let's stick with === and !==.
(I should probably check in an eslint config.)
| } | ||
| if ((name == "min-height") && (newValue != "")) { | ||
| this.minHeight = newValue | ||
| } |
There was a problem hiding this comment.
Some additional logic is required to propagate these changes to the editor.
There was a problem hiding this comment.
I'm not sure I understand what you mean here?
src/faust-editor-basic.ts
Outdated
| import { createEditor, setError, clearError } from "./editor" | ||
| import faustSvg from "./faustText.svg" | ||
|
|
||
| function editorTemplate(readonly: boolean = false, minHeight: string = "") { |
There was a problem hiding this comment.
Looks like readonly is unused in this function.
src/faust-editor-basic.ts
Outdated
|
|
||
| connectedCallback() { | ||
| const code = this.innerHTML.replace("<!--", "").replace("-->", "").trim() | ||
| console.log("connectedCallback: Got %s for min-height", this.minHeight); |
There was a problem hiding this comment.
nit: let's remove the debug logging before this is merged
| border: 1px solid black; | ||
| border-radius: 5px; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
It would be nice to reduce duplication between this module and faust-editor.ts. Perhaps the shared CSS could be moved out to a common module.
There was a problem hiding this comment.
I'd like to keep that for a later task/PR
src/faust-editor-basic.ts
Outdated
| connectedCallback() { | ||
| const code = this.innerHTML.replace("<!--", "").replace("-->", "").trim() | ||
| console.log("connectedCallback: Got %s for min-height", this.minHeight); | ||
| this.attachShadow({ mode: "open" }).appendChild(editorTemplate(this.readonly, this.minHeight).content.cloneNode(true)) |
There was a problem hiding this comment.
Generating a new template node every time defeats the purpose of using a template for cloning; we should just do one or the other.
That is, either generate the innerHTML of the shadow DOM directly without first generating and cloning and template, or clone a fixed template and then tweak it to apply min-height. (I'd prefer the latter.)
There was a problem hiding this comment.
Thanks! To be honest I had no prior experience with writing custom elements, shadow DOM and templates used in that way.
I think if we can get the min-height to be set by the user entirely from the outer document, then I should be able to revert to the initial template.
Afaik though, unnecessary code left aside, the improvement should not be significant unless showing hundreds of editors.
See the readme and the demo page for an example of what it looks like!
I've made this mostly for a Faust paste web service I'm building.
This PR could use some polishing but the claimed features are available and working.
Please do not merge until I've ironed out some details and removed the fork notice in readme.
As for the hack in 0f4b74f to force building 2 variants, ViteJS/Rollup currently don't make this possible and do not seem to plan to support it any time soon ... sigh ... This has been a hugely frustrating waste of time and energy to end up with a stupid workaround, I won't spend more time on this though I'll gladly welcome contributions.