Assessments & Grading Workspace: Add folders mode #2765
Conversation
Pull Request Test Coverage Report for Build 7817133053
💛 - Coveralls |
|
Is this PR ready or still in draft state? |
|
It is PR ready. The draft means we plan to expand on it. |
|
Update: Sorry, it is not PR ready yet. |
…end into read-only-workspace
|
@accountexeregister what is the status of this PR? |
|
Sorry prof, I just saw this message. We will still have to work on the backend before integrating it fully. |
…nto read-only-workspace
Tkaixiang
changed the title
…nto read-only-workspace
| /** | ||
| * @prop fileMode an integer for the mode of the file where | ||
| * 0 = read-only and 1 = read-write. | ||
| */ | ||
| type ControlBarFileModeButtonProps = { | ||
| fileMode: number | null; | ||
| }; |
There was a problem hiding this comment.
Why are we using an integer when we can use an enum?
| export const ControlBarFileModeButton: React.FC<ControlBarFileModeButtonProps> = ({ fileMode }) => { | ||
| if (fileMode === 0) { | ||
| return <ControlButton label={'Read-Only'} icon={IconNames.LOCK} isDisabled />; | ||
| } | ||
| return <ControlButton label={'Read-Write'} icon={IconNames.UNLOCK} isDisabled />; | ||
| }; |
There was a problem hiding this comment.
If the button is always disabled, is a button the most appropriate?
|
|
||
| /* | ||
| ? props.readOnly | ||
| ? 'Evaluation is disabled in read-only mode' | ||
| : '...or press shift-enter in the editor' | ||
| : 'Open a file to evaluate the program with the file as the entrypoint'; | ||
| */ |
There was a problem hiding this comment.
Is this still needed? If not, please remove the commented code.
| isSupportedSource: boolean; | ||
| toggleFolderMode: () => void; | ||
| }; | ||
|
|
||
| export const ControlBarToggleFolderModeButton: React.FC<Props> = ({ | ||
| isFolderModeEnabled, | ||
| isSessionActive, | ||
| isPersistenceActive, | ||
| isSupportedSource, | ||
| toggleFolderMode | ||
| }) => { | ||
| const tooltipContent = isSessionActive | ||
| ? 'Currently unsupported while a collaborative session is active' | ||
| : isPersistenceActive | ||
| ? 'Currently unsupported while a persistence method is active' | ||
| : !isSupportedSource | ||
| ? 'Folder mode is not available for this version of Source' | ||
| : `${isFolderModeEnabled ? 'Disable' : 'Enable'} Folder mode`; | ||
| return ( | ||
| <Tooltip content={tooltipContent}> |
There was a problem hiding this comment.
I'd argue that if the language does not support it, the button should just be hidden (not disabled), like on the Playground.
| @@ -24,6 +25,10 @@ const EditorTab: React.FC<Props> = ({ filePath, isActive, setActive, remove }) = | |||
| })} | |||
| onClick={setActive} | |||
| > | |||
| {' '} | |||
| : editorTab | ||
| ); | ||
|
|
||
| if (isEqual(editorTabs, newEditorTabs)) { |
There was a problem hiding this comment.
Equality does not work properly with drafts. See https://immerjs.github.io/immer/pitfalls/#drafts-arent-referentially-equal
| return { | ||
| ...state, | ||
| [workspaceLocation]: { | ||
| ...state[workspaceLocation], | ||
| editorTabs: newEditorTabs | ||
| } | ||
| }; |
There was a problem hiding this comment.
Try to use mutation instead of returning a new object.
| ); | ||
|
|
||
| if (isEqual(editorTabs, newEditorTabs)) { | ||
| return state; |
There was a problem hiding this comment.
Instead of doing this very expensive computation, just read the old state (O(1) time), and read its readOnly old value (also O(1) time). Then compare it with the payload. If they are equal, return the old state. If they are not equal, compute the new state and return it.
| QuestionTypes, | ||
| Testcase | ||
| } from '../assessment/AssessmentTypes'; | ||
| import { ControlBarProps } from '../controlBar/ControlBar'; | ||
| import { ControlBarChapterSelect } from '../controlBar/ControlBarChapterSelect'; | ||
| import { ControlBarClearButton } from '../controlBar/ControlBarClearButton'; | ||
| import { ControlBarEvalButton } from '../controlBar/ControlBarEvalButton'; | ||
| // import { ControlBarFileModeButton } from '../controlBar/ControlBarFileModeButton'; |
There was a problem hiding this comment.
Commented code should be removed in unused.
| }, []); | ||
| setIsEditable(false); | ||
| dispatch(updateTabReadOnly(workspaceLocation, activeEditorTabIndex, true)); | ||
| }, [activeEditorTabIndex, editorTabs, questionId]); |
There was a problem hiding this comment.
Please fix the lint warnings.
…nto read-only-workspace
Description
To be merged with Grader #319 and Backend #1110
Type of change
Note: This PR might break "Teams" functionality. The
shouldDisableSaveButton()function on line805insrc\commons\assessmentWorkspace\AssessmentWorkspace.tsxcauses the save button to always be disabled in Assessments. Hence we had to comment it out.How to test
Checklist