From e955bccfcf7a724386d0c17f824df87a53ebbb25 Mon Sep 17 00:00:00 2001 From: Oliver Eyton-Williams Date: Thu, 13 Apr 2023 17:25:12 +0200 Subject: [PATCH] refactor: replace isChallenge (#50033) * refactor: replace isChallenge Determining if a path is a challenge by the number of path segments is brittle and we ended up writing bizarre things like isChallenge(nextChallengePath). This should be a little more robust. i.e. if we need to know if a page is a challenge, we can check the challengeMeta * test: update tests with new logic --- client/src/redux/prop-types.ts | 4 +-- .../Challenges/components/hotkeys.tsx | 29 +++++++++++------ .../Challenges/redux/completion-epic.js | 22 +++++-------- client/src/utils/path-parsers.test.ts | 32 +------------------ client/src/utils/path-parsers.ts | 14 -------- client/utils/gatsby/challenge-page-creator.js | 4 +-- client/utils/gatsby/layout-selector.test.tsx | 31 ++++++++++++++---- client/utils/gatsby/layout-selector.tsx | 5 +-- 8 files changed, 60 insertions(+), 81 deletions(-) diff --git a/client/src/redux/prop-types.ts b/client/src/redux/prop-types.ts index 13a4b278336..a6218f2d4e5 100644 --- a/client/src/redux/prop-types.ts +++ b/client/src/redux/prop-types.ts @@ -292,8 +292,8 @@ export type ChallengeMeta = { id: string; introPath: string; isFirstStep: boolean; - nextChallengePath: string; - prevChallengePath: string; + nextChallengePath: string | null; + prevChallengePath: string | null; removeComments: boolean; superBlock: SuperBlocks; title?: string; diff --git a/client/src/templates/Challenges/components/hotkeys.tsx b/client/src/templates/Challenges/components/hotkeys.tsx index db387eb1be4..c4f73732899 100644 --- a/client/src/templates/Challenges/components/hotkeys.tsx +++ b/client/src/templates/Challenges/components/hotkeys.tsx @@ -4,8 +4,12 @@ import { HotKeys, GlobalHotKeys } from 'react-hotkeys'; import { connect } from 'react-redux'; import { createSelector } from 'reselect'; import { editor } from 'monaco-editor'; -import { ChallengeFiles, Test, User } from '../../../redux/prop-types'; -import { isChallenge } from '../../../utils/path-parsers'; +import type { + ChallengeFiles, + Test, + User, + ChallengeMeta +} from '../../../redux/prop-types'; import { userSelector } from '../../../redux/selectors'; import { @@ -57,7 +61,8 @@ const keyMap = { showShortcuts: 'shift+/' }; -interface HotkeysProps { +interface HotkeysProps + extends Pick { canFocusEditor: boolean; challengeFiles: ChallengeFiles; challengeType?: number; @@ -67,8 +72,6 @@ interface HotkeysProps { submitChallenge: () => void; innerRef: MutableRefObject; instructionsPanelRef?: React.RefObject; - nextChallengePath: string; - prevChallengePath: string; setEditorFocusability: (arg0: boolean) => void; setIsAdvancing: (arg0: boolean) => void; tests: Test[]; @@ -137,14 +140,22 @@ function Hotkeys({ navigationMode: () => setEditorFocusability(false), navigatePrev: () => { if (!canFocusEditor) { - if (isChallenge(prevChallengePath)) setIsAdvancing(true); - void navigate(prevChallengePath); + if (prevChallengePath) { + setIsAdvancing(true); + void navigate(prevChallengePath); + } else { + void navigate('/learn'); + } } }, navigateNext: () => { if (!canFocusEditor) { - if (isChallenge(nextChallengePath)) setIsAdvancing(true); - void navigate(nextChallengePath); + if (nextChallengePath) { + setIsAdvancing(true); + void navigate(nextChallengePath); + } else { + void navigate('/learn'); + } } }, showShortcuts: (e: React.KeyboardEvent) => { diff --git a/client/src/templates/Challenges/redux/completion-epic.js b/client/src/templates/Challenges/redux/completion-epic.js index c4dc53c0517..9afa45d9948 100644 --- a/client/src/templates/Challenges/redux/completion-epic.js +++ b/client/src/templates/Challenges/redux/completion-epic.js @@ -10,7 +10,6 @@ import { tap, mergeMap } from 'rxjs/operators'; -import { isChallenge } from '../../../utils/path-parsers'; import { challengeTypes, submitTypes } from '../../../../utils/challenge-types'; import { actionTypes as submitActionTypes } from '../../../redux/action-types'; import { @@ -184,16 +183,19 @@ export default function completionEpic(action$, state$) { submitter = submitters[submitTypes[challengeType]]; } - const pathToNavigateTo = () => { - return findPathToNavigateTo(nextChallengePath, superBlock); - }; + const isNextChallengeInSameSuperBlock = + nextChallengePath.includes(superBlock); + + const pathToNavigateTo = isNextChallengeInSameSuperBlock + ? nextChallengePath + : `/learn/${superBlock}/#${superBlock}-projects`; const canAllowDonationRequest = (state, action) => isBlockNewlyCompletedSelector(state) && action.type === submitActionTypes.submitComplete; return submitter(type, state).pipe( - concat(of(setIsAdvancing(isChallenge(pathToNavigateTo())))), + concat(of(setIsAdvancing(isNextChallengeInSameSuperBlock))), mergeMap(x => canAllowDonationRequest(state, x) ? of(x, allowBlockDonationRequests({ superBlock, block })) @@ -201,7 +203,7 @@ export default function completionEpic(action$, state$) { ), tap(res => { if (res.type !== submitActionTypes.updateFailed) { - navigate(pathToNavigateTo()); + navigate(pathToNavigateTo); } }), concat(of(closeModal('completion'))) @@ -209,11 +211,3 @@ export default function completionEpic(action$, state$) { }) ); } - -function findPathToNavigateTo(nextChallengePath, superBlock) { - if (nextChallengePath.includes(superBlock)) { - return nextChallengePath; - } else { - return `/learn/${superBlock}/#${superBlock}-projects`; - } -} diff --git a/client/src/utils/path-parsers.test.ts b/client/src/utils/path-parsers.test.ts index ad5a002f601..c7e747f082b 100644 --- a/client/src/utils/path-parsers.test.ts +++ b/client/src/utils/path-parsers.test.ts @@ -1,4 +1,4 @@ -import { isChallenge, isLanding } from './path-parsers'; +import { isLanding } from './path-parsers'; const pathnames = { english: { @@ -56,33 +56,3 @@ describe('isLanding', () => { expect(isLanding(pathnames.espanolWithYear.challenge)).toBe(false); }); }); - -describe('isChallenge', () => { - it('returns a boolean', () => { - expect(typeof isChallenge('/')).toBe('boolean'); - }); - it('returns false for Espanol landing pathname', () => { - expect(isChallenge(pathnames.espanol.landing)).toBe(false); - }); - it('returns false for Espanol super block pathname', () => { - expect(isChallenge(pathnames.espanol.superBlock)).toBe(false); - }); - it('returns true for Espanol challenge pathname', () => { - expect(isChallenge(pathnames.espanol.challenge)).toBe(true); - }); - it('returns false for English landing pathname', () => { - expect(isChallenge(pathnames.english.landing)).toBe(false); - }); - it('returns false for English super block pathname', () => { - expect(isChallenge(pathnames.english.superBlock)).toBe(false); - }); - it('returns true for English challenge pathname', () => { - expect(isChallenge(pathnames.english.challenge)).toBe(true); - }); - it('returns true for English with year challenge pathname', () => { - expect(isChallenge(pathnames.englishWithYear.challenge)).toBe(true); - }); - it('returns true for Espanol with year challenge pathname', () => { - expect(isChallenge(pathnames.espanolWithYear.challenge)).toBe(true); - }); -}); diff --git a/client/src/utils/path-parsers.ts b/client/src/utils/path-parsers.ts index 11f23e452db..ac112846793 100644 --- a/client/src/utils/path-parsers.ts +++ b/client/src/utils/path-parsers.ts @@ -4,20 +4,6 @@ import { i18nConstants } from '../../../config/constants'; const splitPath = (pathname: string): string[] => pathname.split('/').filter(x => x); -export const isChallenge = (pathname: string): boolean => { - const pathArray = splitPath(pathname); - return ( - // learn/// - (pathArray.length === 4 && pathArray[0] === 'learn') || - // learn//// - (pathArray.length === 5 && pathArray[0] === 'learn') || - // /learn/// - (pathArray.length === 5 && pathArray[1] === 'learn') || - // /learn//// - (pathArray.length === 6 && pathArray[1] === 'learn') - ); -}; - export const isLanding = (pathname: string): boolean => { const pathArray = splitPath(pathname); const isEnglishLanding = pathArray.length === 0; diff --git a/client/utils/gatsby/challenge-page-creator.js b/client/utils/gatsby/challenge-page-creator.js index 2d342bc4965..8ef899c4293 100644 --- a/client/utils/gatsby/challenge-page-creator.js +++ b/client/utils/gatsby/challenge-page-creator.js @@ -58,12 +58,12 @@ function getIsFirstStep(_node, index, nodeArray) { function getNextChallengePath(_node, index, nodeArray) { const next = nodeArray[index + 1]; - return next ? next.node.challenge.fields.slug : '/learn'; + return next ? next.node.challenge.fields.slug : null; } function getPrevChallengePath(_node, index, nodeArray) { const prev = nodeArray[index - 1]; - return prev ? prev.node.challenge.fields.slug : '/learn'; + return prev ? prev.node.challenge.fields.slug : null; } function getTemplateComponent(challengeType) { diff --git a/client/utils/gatsby/layout-selector.test.tsx b/client/utils/gatsby/layout-selector.test.tsx index f56a25a68fb..c3c2d4965f1 100644 --- a/client/utils/gatsby/layout-selector.test.tsx +++ b/client/utils/gatsby/layout-selector.test.tsx @@ -19,7 +19,8 @@ interface NameAndProps { } function getComponentNameAndProps( elementType: React.JSXElementConstructor, - pathname: string + pathname: string, + pageContext?: { challengeMeta?: { block?: string; superBlock?: string } } ): NameAndProps { // eslint-disable-next-line testing-library/render-result-naming-convention const shallow = ShallowRenderer.createRenderer(); @@ -28,7 +29,8 @@ function getComponentNameAndProps( props: { location: { pathname - } + }, + pageContext } }); shallow.render({LayoutReactComponent}); @@ -44,10 +46,21 @@ function getComponentNameAndProps( }; } -test('Challenge path should have DefaultLayout and no footer', () => { +const challengePageContext = { + challengeMeta: { + block: 'Basic HTML and HTML5', + superBlock: 'responsive-web-design' + } +}; + +test('Challenges should have DefaultLayout and no footer', () => { const challengePath = '/learn/responsive-web-design/basic-html-and-html5/say-hello-to-html-elements'; - const compnentObj = getComponentNameAndProps(Learn, challengePath); + const compnentObj = getComponentNameAndProps( + Learn, + challengePath, + challengePageContext + ); expect(compnentObj.name).toEqual('DefaultLayout'); expect(compnentObj.props.showFooter).toEqual(false); }); @@ -59,15 +72,19 @@ test('SuperBlock path should have DefaultLayout and footer', () => { expect(compnentObj.props.showFooter).toEqual(true); }); -test('i18l challenge path should have DefaultLayout and no footer', () => { +test('i18n challenge path should have DefaultLayout and no footer', () => { const challengePath = 'espanol/learn/responsive-web-design/basic-html-and-html5/say-hello-to-html-elements/'; - const compnentObj = getComponentNameAndProps(Learn, challengePath); + const compnentObj = getComponentNameAndProps( + Learn, + challengePath, + challengePageContext + ); expect(compnentObj.name).toEqual('DefaultLayout'); expect(compnentObj.props.showFooter).toEqual(false); }); -test('i18l superBlock path should have DefaultLayout and footer', () => { +test('i18n superBlock path should have DefaultLayout and footer', () => { const superBlockPath = '/learn/responsive-web-design/'; const compnentObj = getComponentNameAndProps(Learn, superBlockPath); expect(compnentObj.name).toEqual('DefaultLayout'); diff --git a/client/utils/gatsby/layout-selector.tsx b/client/utils/gatsby/layout-selector.tsx index ac9f7df4beb..aa72727168c 100644 --- a/client/utils/gatsby/layout-selector.tsx +++ b/client/utils/gatsby/layout-selector.tsx @@ -3,7 +3,6 @@ import React from 'react'; import CertificationLayout from '../../src/components/layouts/certification'; import DefaultLayout from '../../src/components/layouts/default'; import FourOhFourPage from '../../src/pages/404'; -import { isChallenge } from '../../src/utils/path-parsers'; interface LayoutSelectorProps { element: JSX.Element; @@ -20,6 +19,8 @@ export default function layoutSelector({ location: { pathname } } = props; + const isChallenge = !!props.pageContext?.challengeMeta; + if (element.type === FourOhFourPage) { return ( @@ -30,7 +31,7 @@ export default function layoutSelector({ return ( {element} ); - } else if (isChallenge(pathname)) { + } else if (isChallenge) { return (