-
Notifications
You must be signed in to change notification settings - Fork 169
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
feat: add vscode slice for message passing with extension #3080
base: master
Are you sure you want to change the base?
Conversation
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.
Hi, thanks for working on this! Could you update the test snapshots?
Pull Request Test Coverage Report for Build 13177704939Details
💛 - Coveralls |
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.
Just some minor nits :)
// useEffect(() => { | ||
// // TODO: Hardcoded to make use of the first editor tab. Refactoring is needed for this workspace to enable Folder mode. | ||
// handleEditorValueChange(0, ''); | ||
// // eslint-disable-next-line react-hooks/exhaustive-deps | ||
// }, []); |
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.
Will this break anything with assessments? Have you tested locally?
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.
From my local testing, there's no unintended behavior. Assessments still loading as per normal.
handleEditorValueChange is ultimately called with the editor contents due to the useEffect here.
src/commons/workspace/Workspace.tsx
Outdated
@@ -222,7 +223,11 @@ const Workspace: React.FC<WorkspaceProps> = props => { | |||
</Resizable> | |||
<div className="row content-parent" ref={contentContainerDiv}> | |||
<div className="editor-divider" ref={editorDividerDiv} /> | |||
<Resizable {...editorResizableProps()}>{createWorkspaceInput(props)}</Resizable> | |||
{isVscode ? ( | |||
<div style={{ width: '0px' }}>{createWorkspaceInput(props)}</div> |
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 must still be visible is it? We can't use visibility: hidden
? If we can't, then in addition to width: 0
, should we also use overflow: hidden
?
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.
While visibility: hidden
wouldn't work, I think we can completely remove it now that messages are passed directly to the redux store rather than to the editor.
Co-authored-by: Richard Dominick <[email protected]>
Description
FYP: Source Academy as Visual Studio Code Extension
This PR makes the frontend's behavior change conditionally when it detects itself to be running as an iframe in the VS Code extension. This is required so that it can send and receive the editor text contents, encapsulated as messages, to the VS Code extension.
Type of change
How to test
Checklist