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

Combine the editor instances #12

Closed
domoritz opened this issue Jan 19, 2018 · 10 comments
Closed

Combine the editor instances #12

domoritz opened this issue Jan 19, 2018 · 10 comments

Comments

@domoritz
Copy link
Member

Right now we create new monaco instances when we open the vega spec in vl mode. This prevents undo/redo.

@domoritz
Copy link
Member Author

Just to explain this a bit more. The problem is that react recreates components when a subtree of the dom changes. This means we recreate the monaco editor instance in some case and erase the (undo) history.

@domoritz
Copy link
Member Author

domoritz commented Feb 20, 2018

We have fixed the issue when the Vega editor instance is opened side by side with Vega-Lite. However, switching from Vega to Vega-Lite and vice versa or opening examples still creates a new editor.

@algomaster99
Copy link
Member

@domoritz On switching from vega to vega-lite, we can just change the editorString right? This is how this issue can be solved right?

@domoritz
Copy link
Member Author

Yeah, maybe. The issue is that react will re-create the component instead of reusing the editor we already have. This is one of the most important issues we have right now. I would love to support undo across different modes for example.

@algomaster99
Copy link
Member

Oh okay. You mean undo can be done even if we switch editors?

@algomaster99 algomaster99 self-assigned this Mar 12, 2019
@domoritz
Copy link
Member Author

It can't right now because we create a new Monaco instance.

@algomaster99
Copy link
Member

Yeah right. I'll try to fix this issue.
What I am thinking is that we just change editorStrings. This will prevent from creating a new instance and also undoing will be possible. I'll look into it and let you know the progress soon

@domoritz
Copy link
Member Author

Great. It's a bit tricky to do but shouldn't be impossible.

@algomaster99 algomaster99 removed their assignment Mar 29, 2019
@algomaster99
Copy link
Member

I have unassigned this issue to myself so others can also take a look.

@domoritz
Copy link
Member Author

We support undo/redo across modes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants