-
Notifications
You must be signed in to change notification settings - Fork 108
remove monaco in favor of codemirror #2920
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
const createEditorView = () => { | ||
return new EditorView({ | ||
parent: editorElement, |
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.
You could make this function pure if you passed editorElement in instead of depending on the state being declared already.
return new EditorView({ | ||
parent: editorElement, | ||
state: EditorState.create({ | ||
doc: content, |
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.
Same here but unsure if EditorState updates content, but you could purify the func by passing this in as well, the rest seem to be options
if (transaction.docChanged) { | ||
content = editorView.state.doc.toString(); | ||
} | ||
}, |
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.
Lol I got to here and it seems less feasible but I'm curious when this function ends up getting called in the lifecycle of codemirror.
Probably ignore the comments above.
await tick(); | ||
editorView = createEditorView(); | ||
} | ||
}; |
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.
If you made an editor component and used it in the if statement wouldn't this happen automagically without you having to manage and use await tick? Also the onMount would naturally move to the new editor component.
<section class={twMerge('h-full w-full', className)}> | ||
<iframe | ||
bind:this={iframe} | ||
onload={resizeIframe} |
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.
Do we care if the window size is changed and need to update this? Should we use a resizeObserver (or whatever the window version is) on this to keep things properly laid out?
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.
Good question, this was existing code I copied over from src/lib/holocene/monaco/markdown.svelte, so I didn't want to touch it.
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.
It seems to work fine
Screen.Recording.2025-09-18.at.1.04.08.PM.mov
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.
The implementation is looking good. I left a couple suggestions for the component, and a request for a storybook story so I can use the preview link to try the component :)
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.
LGTM
Description & motivation 💭
Remove monaco-editor and use codemirror instead for nexus desccription
Screenshots (if applicable) 📸
Screen.Recording.2025-09-18.at.11.22.30.AM.mov
Design Considerations 🎨
Testing 🧪
How was this tested 👻
Steps for others to test: 🚶🏽♂️🚶🏽♀️
Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
Docs
Any docs updates needed?