-
Notifications
You must be signed in to change notification settings - Fork 172
Segregate Playground and Assessment Workspaces #137
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
I will add this back as an OwnProps property, passed by either Assessment or Playground
Added an additional parameter to denote the location of the action call.
Due to the workspace's input and dispatch being determined by the context, they can be passed in as props by Assessment or Playground.
Also fixed some errors picked up at compilation time.
For some reason the member-ordering linting does not pass despite proper(?) ordering. I'll open an issue for this.
This holds the 2 instances of IWorkspaceState, playground and assessment. I've also added a scaffold of how the reducer function would work, given a location. The location (type WorkspaceLocation) is now specified to correspond to the key of the IWorkspaceState within the IWorkspaceManagerState.
Also made some interpreter actions to include location
- Removed cached editorValue - Renamed IState.workspace into workspaces - Fixed a typo
Also added a missing handleChangeActiveTab
For some reason, the spread and rest operators did not work with the varying argument lengths. I proceeded to change them to a manual currying. Also fixed a bug with the argument type for handleEditorWidthChange.
src/actions/interpreter.ts
Outdated
type: actionTypes.HANDLE_CONSOLE_LOG, | ||
payload: log | ||
payload: { log, workspaceLocation } |
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.
logString
is more descriptive imo
src/actions/workspace.ts
Outdated
* Note that the names must correspond with the name of the | ||
* object in IWorkspaceManagerState. | ||
*/ | ||
export type WorkspaceLocation = 'assessment' | 'playground' |
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.
Can you use values from the enum instead of string literals?
src/components/Playground.tsx
Outdated
editorWidth: string | ||
output: InterpreterOutput[] | ||
replValue: string | ||
} |
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.
Quite a lot of properties here, I think they could be alphabeticalised
Thanks for the quick review, have applied the changes 👍. I'll start on the other features now. |
This allows the "presetting" of values like in the case of a solutionTemplate. However, it also forces any component to provide such a value, thus I have added a template string for playground.
- For IStateProps, the properties I was assigning to were optional, meaning that the linting did not catch the error. I overcame this by storing and casting the return value. - Made a mistake in using es6 object literal notation in the reducer, as the property name was not as expected.
@ningyuansg I've added the "reset" feature. Do you think I could afford to keep |
src/reducers/states.ts
Outdated
editorWidth: '50%', | ||
isRunning: false, | ||
output: [], | ||
replValue: '', | ||
sideContentActiveTab: 0, | ||
sideContentHeight: undefined, | ||
sideContentHeight: 60, |
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.
Does this cause overflow? A fresh workspace should render such that the side content takes up a size that is just big enough to prevent overflow of its contents.
src/sagas/index.ts
Outdated
@@ -15,7 +16,7 @@ function* mainSaga() { | |||
yield* apiFetchSaga() | |||
yield* interpreterSaga() |
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.
Can consider renaming as workspaceSaga
Properties are more readable imo. Not sure about efficiency. |
In line with the naming in the state and reducer.
Changes made! @ningyuansg |
Really big PR, gonna merge this in first. |
Features
TODO
display
function needs to pass in alocation
argument, to be kept in the Context