fix: stop modal appearing in steps (#43728)

* fix: stop showing completion modal on steps

* feat: submit steps with ctrl+enter

* fix: handle ctrl+enter when not focussing editor

* fix: reset tests when user types

* refactor: pass showCompletionModal as an option

Otherwise we have to write executeChallenge(true) which does not mean
what you might reasonably expect.

* fix: always executeChallenge when not on step

* fix: update frontend project show

* fix: handle missing payload

* refactor: isProjectStep -> hasEditableRegion

* refactor: more renaming

* fix: make meta.json control multifile editor use

* fix: update the challengeSchema correctly

* Update client/src/templates/Challenges/classic/editor.tsx

Co-authored-by: Shaun Hamilton <shauhami020@gmail.com>

* fix: remove logging

Co-authored-by: Shaun Hamilton <shauhami020@gmail.com>

Co-authored-by: Shaun Hamilton <shauhami020@gmail.com>
This commit is contained in:
Oliver Eyton-Williams
2021-10-13 13:47:59 +02:00
committed by GitHub
parent 9220bfedad
commit 22afdd1aad
24 changed files with 149 additions and 45 deletions

View File

@ -223,6 +223,7 @@ export type ChallengeNodeType = {
title: string;
translationPending: boolean;
url: string;
usesMultifileEditor: boolean;
videoId: string;
videoLocaleIds?: VideoLocaleIds;
bilibiliIds?: BilibiliIds;
@ -436,6 +437,7 @@ export type ChallengeFile = {
ext: ExtTypes;
name: string;
editableRegionBoundaries: number[];
usesMultifileEditor: boolean;
path: string;
error: null | string;
head: string;

View File

@ -31,6 +31,7 @@ const propTypes = {
fileKey: PropTypes.string,
initialEditorContent: PropTypes.string,
initialExt: PropTypes.string,
initialTests: PropTypes.array,
output: PropTypes.arrayOf(PropTypes.string),
resizeProps: PropTypes.shape({
onStopResize: PropTypes.func,
@ -42,6 +43,7 @@ const propTypes = {
// TODO: is this used?
title: PropTypes.string,
updateFile: PropTypes.func.isRequired,
usesMultifileEditor: PropTypes.bool,
visibleEditors: PropTypes.shape({
indexjs: PropTypes.bool,
indexjsx: PropTypes.bool,
@ -84,10 +86,12 @@ class MultifileEditor extends Component {
containerRef,
description,
editorRef,
initialTests,
theme,
resizeProps,
title,
visibleEditors: { indexcss, indexhtml, indexjs, indexjsx }
visibleEditors: { indexcss, indexhtml, indexjs, indexjsx },
usesMultifileEditor
} = this.props;
const editorTheme = theme === 'night' ? 'vs-dark-custom' : 'vs-custom';
// TODO: the tabs mess up the rendering (scroll doesn't work properly and
@ -139,10 +143,12 @@ class MultifileEditor extends Component {
description={targetEditor === 'indexjsx' ? description : null}
editorRef={editorRef}
fileKey='indexjsx'
initialTests={initialTests}
key='indexjsx'
resizeProps={resizeProps}
theme={editorTheme}
title={title}
usesMultifileEditor={usesMultifileEditor}
/>
</ReflexElement>
)}
@ -159,10 +165,12 @@ class MultifileEditor extends Component {
}
editorRef={editorRef}
fileKey='indexhtml'
initialTests={initialTests}
key='indexhtml'
resizeProps={resizeProps}
theme={editorTheme}
title={title}
usesMultifileEditor={usesMultifileEditor}
/>
</ReflexElement>
)}
@ -177,10 +185,12 @@ class MultifileEditor extends Component {
description={targetEditor === 'indexcss' ? description : null}
editorRef={editorRef}
fileKey='indexcss'
initialTests={initialTests}
key='indexcss'
resizeProps={resizeProps}
theme={editorTheme}
title={title}
usesMultifileEditor={usesMultifileEditor}
/>
</ReflexElement>
)}
@ -196,10 +206,12 @@ class MultifileEditor extends Component {
description={targetEditor === 'indexjs' ? description : null}
editorRef={editorRef}
fileKey='indexjs'
initialTests={initialTests}
key='indexjs'
resizeProps={resizeProps}
theme={editorTheme}
title={title}
usesMultifileEditor={usesMultifileEditor}
/>
</ReflexElement>
)}

View File

@ -37,7 +37,8 @@ import {
setEditorFocusability,
updateFile,
challengeTestsSelector,
submitChallenge
submitChallenge,
initTests
} from '../redux';
import './editor.css';
@ -52,11 +53,14 @@ interface EditorProps {
description: string;
dimensions: DimensionsType;
editorRef: MutableRefObject<editor.IStandaloneCodeEditor>;
executeChallenge: (isShouldCompletionModalOpen?: boolean) => void;
executeChallenge: (options?: { showCompletionModal: boolean }) => void;
ext: ExtTypes;
fileKey: FileKeyTypes;
initialEditorContent: string;
initialExt: string;
initTests: (tests: Test[]) => void;
initialTests: Test[];
isProjectStep: boolean;
output: string[];
resizeProps: ResizePropsType;
saveEditorContent: () => void;
@ -70,6 +74,7 @@ interface EditorProps {
editorValue: string;
editableRegionBoundaries: number[] | null;
}) => void;
usesMultifileEditor: boolean;
}
interface EditorProperties {
@ -122,7 +127,8 @@ const mapDispatchToProps = {
saveEditorContent,
setEditorFocusability,
updateFile,
submitChallenge
submitChallenge,
initTests
};
const modeMap = {
@ -181,7 +187,7 @@ const initialData: EditorProperties = {
};
const Editor = (props: EditorProps): JSX.Element => {
const { editorRef, fileKey } = props;
const { editorRef, fileKey, initTests } = props;
// These refs are used during initialisation of the editor as well as by
// callbacks. Since they have to be initialised before editorWillMount and
// editorDidMount are called, we cannot use useState. Reason being that will
@ -198,6 +204,11 @@ const Editor = (props: EditorProps): JSX.Element => {
});
const data = dataRef.current[fileKey];
// since editorDidMount runs once with the initial props object, it keeps a
// reference to *those* props. If we want it to use the latest props, we can
// use a ref, since it will be updated on every render.
const testRef = useRef<Test[]>([]);
testRef.current = props.tests;
// TENATIVE PLAN: create a typical order [html/jsx, css, js], put the
// available files into that order. i.e. if it's just one file it will
@ -294,7 +305,7 @@ const Editor = (props: EditorProps): JSX.Element => {
data.editor = editor;
if (hasEditableRegion()) {
initializeProjectStepFeatures();
initializeDescriptionAndOutputWidgets();
addContentChangeListener();
showEditableRegion(editor);
}
@ -342,8 +353,17 @@ const Editor = (props: EditorProps): JSX.Element => {
label: 'Run tests',
/* eslint-disable no-bitwise */
keybindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.Enter],
// TODO: Discuss with Ahmad what should pop-up when a challenge is completed
run: () => props.executeChallenge(true)
run: () => {
if (props.usesMultifileEditor) {
if (challengeIsComplete()) {
props.submitChallenge();
} else {
props.executeChallenge();
}
} else {
props.executeChallenge({ showCompletionModal: true });
}
}
});
editor.addAction({
id: 'leave-editor',
@ -648,7 +668,7 @@ const Editor = (props: EditorProps): JSX.Element => {
}
};
function initializeProjectStepFeatures() {
function initializeDescriptionAndOutputWidgets() {
const editor = data.editor;
if (editor) {
initializeRegions(getEditableRegionFromRedux());
@ -955,6 +975,17 @@ const Editor = (props: EditorProps): JSX.Element => {
.setEndPosition(range.endLineNumber, endColumnText.length + 2);
}
function challengeIsComplete() {
const tests = testRef.current;
return tests.every(test => test.pass && !test.err);
}
function challengeHasErrors() {
const tests = testRef.current;
return tests.some(test => test.err);
}
// runs every update to the editor and when the challenge is reset
useEffect(() => {
// If a challenge is reset, it needs to communicate that change to the
// editor.
@ -962,13 +993,15 @@ const Editor = (props: EditorProps): JSX.Element => {
const hasChangedContents = updateEditorValues();
if (hasChangedContents && hasEditableRegion()) {
initializeProjectStepFeatures();
initializeDescriptionAndOutputWidgets();
updateDescriptionZone();
updateOutputZone();
}
if (hasChangedContents && !hasEditableRegion()) editor?.focus();
if (props.initialTests) initTests(props.initialTests);
if (hasEditableRegion() && editor) {
if (hasChangedContents) {
editor.focus();
@ -1006,14 +1039,12 @@ const Editor = (props: EditorProps): JSX.Element => {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.challengeFiles]);
useEffect(() => {
const { output, tests } = props;
const { output } = props;
const editableRegion = getEditableRegionFromRedux();
if (editableRegion.length === 2) {
const challengeComplete = tests.every(test => test.pass && !test.err);
const chellengeHasErrors = tests.some(test => test.err);
const testOutput = document.getElementById('test-output');
const testStatus = document.getElementById('test-status');
if (challengeComplete) {
if (challengeIsComplete()) {
const testButton = document.getElementById('test-button');
if (testButton) {
testButton.innerHTML =
@ -1038,7 +1069,7 @@ const Editor = (props: EditorProps): JSX.Element => {
testOutput.innerHTML = '';
testStatus.innerHTML = '&#9989; Step completed.';
}
} else if (chellengeHasErrors && testStatus && testOutput) {
} else if (challengeHasErrors() && testStatus && testOutput) {
const wordsArray = [
"Not quite. Here's a hint:",
'Try again. This might help:',

View File

@ -79,7 +79,7 @@ interface ShowClassicProps {
challengeMounted: (arg0: string) => void;
createFiles: (arg0: ChallengeFile[]) => void;
data: { challengeNode: ChallengeNodeType };
executeChallenge: () => void;
executeChallenge: (options?: { showCompletionModal: boolean }) => void;
challengeFiles: ChallengeFiles;
initConsole: (arg0: string) => void;
initTests: (tests: Test[]) => void;
@ -315,7 +315,15 @@ class ShowClassic extends Component<ShowClassicProps, ShowClassicState> {
}
renderEditor() {
const { challengeFiles } = this.props;
const {
challengeFiles,
data: {
challengeNode: {
fields: { tests },
usesMultifileEditor
}
}
} = this.props;
const { description, title } = this.getChallenge();
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return (
@ -326,8 +334,10 @@ class ShowClassic extends Component<ShowClassicProps, ShowClassicState> {
description={description}
editorRef={this.editorRef}
hasEditableBoundries={this.hasEditableBoundries()}
initialTests={tests}
resizeProps={this.resizeProps}
title={title}
usesMultifileEditor={usesMultifileEditor}
/>
)
);
@ -368,7 +378,8 @@ class ShowClassic extends Component<ShowClassicProps, ShowClassicState> {
fields: { blockName },
forumTopicId,
superBlock,
title
title,
usesMultifileEditor
} = this.getChallenge();
const {
executeChallenge,
@ -387,6 +398,7 @@ class ShowClassic extends Component<ShowClassicProps, ShowClassicState> {
instructionsPanelRef={this.instructionsPanelRef}
nextChallengePath={nextChallengePath}
prevChallengePath={prevChallengePath}
usesMultifileEditor={usesMultifileEditor}
>
<LearnLayout>
<Helmet
@ -474,6 +486,7 @@ export const query = graphql`
link
src
}
usesMultifileEditor
challengeFiles {
fileKey
ext

View File

@ -5,18 +5,29 @@ import React from 'react';
import { HotKeys, GlobalHotKeys } from 'react-hotkeys';
import { connect } from 'react-redux';
import { createSelector } from 'reselect';
import { ChallengeFiles, Test } from '../../../redux/prop-types';
import { canFocusEditorSelector, setEditorFocusability } from '../redux';
import {
canFocusEditorSelector,
setEditorFocusability,
challengeFilesSelector,
submitChallenge,
challengeTestsSelector
} from '../redux';
import './hotkeys.css';
const mapStateToProps = createSelector(
canFocusEditorSelector,
(canFocusEditor: boolean) => ({
canFocusEditor
challengeFilesSelector,
challengeTestsSelector,
(canFocusEditor: boolean, challengeFiles: ChallengeFiles, tests: Test[]) => ({
canFocusEditor,
challengeFiles,
tests
})
);
const mapDispatchToProps = { setEditorFocusability };
const mapDispatchToProps = { setEditorFocusability, submitChallenge };
const keyMap = {
NAVIGATION_MODE: 'escape',
@ -29,15 +40,19 @@ const keyMap = {
interface HotkeysProps {
canFocusEditor: boolean;
challengeFiles: ChallengeFiles;
children: React.ReactElement;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
editorRef?: React.Ref<HTMLElement> | any;
executeChallenge?: () => void;
executeChallenge?: (options?: { showCompletionModal: boolean }) => void;
submitChallenge: () => void;
innerRef: React.Ref<HTMLElement> | unknown;
instructionsPanelRef?: React.RefObject<HTMLElement>;
nextChallengePath: string;
prevChallengePath: string;
setEditorFocusability: (arg0: boolean) => void;
tests: Test[];
usesMultifileEditor?: boolean;
}
function Hotkeys({
@ -49,7 +64,10 @@ function Hotkeys({
innerRef,
nextChallengePath,
prevChallengePath,
setEditorFocusability
setEditorFocusability,
submitChallenge,
tests,
usesMultifileEditor
}: HotkeysProps): JSX.Element {
const handlers = {
EXECUTE_CHALLENGE: (e: React.KeyboardEvent<HTMLButtonElement>) => {
@ -58,7 +76,20 @@ function Hotkeys({
// TODO: 'enter' on its own also disables HotKeys, but default behaviour
// should not be prevented in that case.
e.preventDefault();
if (executeChallenge) executeChallenge();
if (!executeChallenge) return;
const testsArePassing = tests.every(test => test.pass && !test.err);
if (usesMultifileEditor) {
if (testsArePassing) {
submitChallenge();
} else {
executeChallenge();
}
} else {
executeChallenge({ showCompletionModal: true });
}
},
FOCUS_EDITOR: (e: React.KeyboardEvent) => {
e.preventDefault();

View File

@ -44,7 +44,7 @@ function ToolPanel({
videoUrl
}) {
const handleRunTests = () => {
executeChallenge(true);
executeChallenge({ showCompletionModal: true });
};
const { t } = useTranslation();
return (

View File

@ -75,7 +75,7 @@ interface BackEndProps {
challengeMounted: (arg0: string) => void;
data: { challengeNode: ChallengeNodeType };
description: string;
executeChallenge: (arg0: boolean) => void;
executeChallenge: (options: { showCompletionModal: boolean }) => void;
forumTopicId: number;
id: string;
initConsole: () => void;
@ -169,11 +169,13 @@ class BackEnd extends Component<BackEndProps> {
}
handleSubmit({
isShouldCompletionModalOpen
showCompletionModal
}: {
isShouldCompletionModalOpen: boolean;
showCompletionModal: boolean;
}): void {
this.props.executeChallenge(isShouldCompletionModalOpen);
this.props.executeChallenge({
showCompletionModal
});
}
render() {

View File

@ -120,11 +120,11 @@ class Project extends Component<ProjectProps> {
}
handleSubmit({
isShouldCompletionModalOpen
showCompletionModal
}: {
isShouldCompletionModalOpen: boolean;
showCompletionModal: boolean;
}): void {
if (isShouldCompletionModalOpen) {
if (showCompletionModal) {
this.props.openCompletionModal();
}
}

View File

@ -11,7 +11,7 @@ import {
import { Form } from '../../../components/formHelpers';
interface SubmitProps {
isShouldCompletionModalOpen: boolean;
showCompletionModal: boolean;
}
interface FormProps extends WithTranslation {
@ -43,9 +43,9 @@ export class SolutionForm extends Component<FormProps> {
// updates values on store
this.props.updateSolutionForm(validatedValues.values);
if (validatedValues.invalidValues.length === 0) {
this.props.onSubmit({ isShouldCompletionModalOpen: true });
this.props.onSubmit({ showCompletionModal: true });
} else {
this.props.onSubmit({ isShouldCompletionModalOpen: false });
this.props.onSubmit({ showCompletionModal: false });
}
}
};

View File

@ -47,7 +47,7 @@ export function* executeCancellableChallengeSaga(payload) {
if (previewTask) {
yield cancel(previewTask);
}
// executeChallenge with payload containing isShouldCompletionModalOpen
// executeChallenge with payload containing {showCompletionModal}
const task = yield fork(executeChallengeSaga, payload);
previewTask = yield fork(previewChallengeSaga, { flushLogs: false });
@ -59,9 +59,7 @@ export function* executeCancellablePreviewSaga() {
previewTask = yield fork(previewChallengeSaga);
}
export function* executeChallengeSaga({
payload: isShouldCompletionModalOpen
}) {
export function* executeChallengeSaga({ payload }) {
const isBuildEnabled = yield select(isBuildEnabledSelector);
if (!isBuildEnabled) {
return;
@ -99,7 +97,7 @@ export function* executeChallengeSaga({
yield put(updateTests(testResults));
const challengeComplete = testResults.every(test => test.pass && !test.err);
if (challengeComplete && isShouldCompletionModalOpen) {
if (challengeComplete && payload?.showCompletionModal) {
yield put(openModal('completion'));
}

View File

@ -1,6 +1,7 @@
{
"name": "Accessibility Quiz",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "accessibility-quiz",
"order": 42,
"time": "5 hours",
@ -279,4 +280,4 @@
"Part 67"
]
]
}
}

View File

@ -1,6 +1,7 @@
{
"name": "Basic CSS Cafe Menu",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "basic-css-cafe-menu",
"order": 10,
"time": "5 hours",

View File

@ -1,6 +1,7 @@
{
"name": "Basic HTML Cat Photo App",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "basic-html-cat-photo-app",
"order": 9,
"time": "5 hours",

View File

@ -1,6 +1,7 @@
{
"name": "Basic JavaScript RPG Game",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "basic-javascript-rpg-game",
"order": 11,
"time": "2 hours",

View File

@ -1,6 +1,7 @@
{
"name": "CSS Box Model",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "css-box-model",
"order": 12,
"time": "5 hours",

View File

@ -1,6 +1,7 @@
{
"name": "CSS Piano",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "css-piano",
"order": 13,
"time": "5 hours",

View File

@ -1,6 +1,7 @@
{
"name": "CSS Picasso Painting",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "css-picasso-painting",
"order": 11,
"time": "5 hours",
@ -367,4 +368,4 @@
"Part 89"
]
]
}
}

View File

@ -1,6 +1,7 @@
{
"name": "CSS Variables Skyline",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "css-variables-skyline",
"order": 8,
"time": "5 hours",

View File

@ -1,6 +1,7 @@
{
"name": "D3 Dashboard",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "d3-dashboard",
"order": 4,
"time": "5 hours",

View File

@ -1,6 +1,7 @@
{
"name": "Functional Programming Spreadsheet",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "functional-programming-spreadsheet",
"order": 13,
"time": "2 hours",

View File

@ -1,6 +1,7 @@
{
"name": "Intermediate JavaScript Calorie Counter",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "intermediate-javascript-calorie-counter",
"order": 12,
"time": "2 hours",

View File

@ -287,7 +287,8 @@ ${getFullPath('english')}
isPrivate,
required = [],
template,
time
time,
usesMultifileEditor
} = meta;
challenge.block = dasherize(blockName);
challenge.order = order;
@ -302,6 +303,7 @@ ${getFullPath('english')}
challenge.helpCategory || helpCategoryMap[challenge.block];
challenge.translationPending =
lang !== 'english' && !isAuditedCert(lang, superBlock);
challenge.usesMultifileEditor = !!usesMultifileEditor;
return prepareChallenge(challenge);
}

View File

@ -111,7 +111,8 @@ const schema = Joi.object()
url: Joi.when('challengeType', {
is: challengeTypes.codeally,
then: Joi.string().required()
})
}),
usesMultifileEditor: Joi.boolean()
})
.xor('helpCategory', 'isPrivate');

View File

@ -1,6 +1,7 @@
{
"name": "",
"isUpcomingChange": true,
"usesMultifileEditor": true,
"dashedName": "",
"order": 42,
"time": "5 hours",