-
Notifications
You must be signed in to change notification settings - Fork 172
Add external library selection #194
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
Only injected the sounds.js file
And also make sourceLibraries definition more concise
Pull Request Test Coverage Report for Build 296
💛 - Coveralls |
And fix an action typo
And also fix some bugs
This was supposed to use to the same one as Assessment, but was not updated. It has been upated now, my bad.
Note that the changes in Grading already reflect this
This is to allow easier reset of workspaces
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.
Your CI is failing as well. https://travis-ci.org/source-academy/cadet-frontend/builds/403438035
There's a button to restart the travis build on the top right of the travis website.
src/components/Playground.tsx
Outdated
@@ -68,7 +71,8 @@ class Playground extends React.Component<IPlaygroundProps, PlaygroundState> { | |||
hasShareButton: true, | |||
isRunning: this.props.isRunning, | |||
queryString: this.props.queryString, | |||
sourceChapter: this.props.sourceChapter | |||
sourceChapter: this.props.sourceChapter, | |||
sourceLibrary: this.props.sourceLibrary |
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.
I really think this should be renamed externalLibrary
. If no objections, please remember to change the directory name under ./public/ where these libraries are stored, as well as the state property, etc.
* Checks if there is a need to reset the workspace, then executes | ||
* a dispatch (in the props) if needed. | ||
* | ||
* @param props the props passed to 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.
Not a useful param tag can it be removed? Description at line 118 is fine, though.
@@ -4,6 +4,7 @@ import { | |||
CHANGE_ACTIVE_TAB, | |||
CHANGE_CHAPTER, | |||
CHANGE_EDITOR_WIDTH, | |||
CHANGE_LIBRARY, |
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.
CHANGE_EXTERNALS, since the prop name to workspace is externals. Mirrors CHANGE_CHAPTER for prop name of chapter.
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.
As I'm going to be adding externals
into the state in another PR, is it alright if I handle it there? As of now, this semantically makes sense as I change the playgroundLibrary
property in the state.
handleChapterSelect?: (i: IChapter, e: React.ChangeEvent<HTMLSelectElement>) => void | ||
handleLibrarySelect?: (i: ILibrary, e: React.ChangeEvent<HTMLSelectElement>) => void |
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.
Prefer sourceExternals and handleExternalsSelect.
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.
As discussed as above, I can incorporate this as well since they are the change in naming.
tsconfig.json
Outdated
@@ -19,6 +19,7 @@ | |||
}, | |||
"exclude": [ | |||
"node_modules", | |||
"public/sourceLibs", |
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.
externalLibs, or external_lib since we're dealing with URLs
Features
make_sourcesound
,array_test
anddraw_connected
added as externalsDependencies
externals: string[]
support (0.1.3)