-
Notifications
You must be signed in to change notification settings - Fork 17
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
Sync editor upon editor change #448
Conversation
for more information, see https://pre-commit.ci
Integration tests repot: appsharing.space |
Preview PR at appsharing.space |
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.
Neat!
I don't quite like that this lives under commands.ts
though. I'd suggest we put this code somewhere in the JupyterCadPanel
class (under packages/base/src/widget.ts
)
I agree with @martinRenou |
Thanks both for suggesting this, i hope I did it alrighty in |
With your recent changes you are still calling this logic in This may be a bit more involved as you'd need for the This way the JupyterCadPanel is responsible for resizing itself when the context changes. |
Thanks for the comprehensive explanation Martin. This sounds good, i'll be on 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.
Neat! I just have a final suggestion
Thanks! |
Ah weird, the merge commit does not pass the integration tests. |
Yes, should be related to #471 |
Should close #450