From d9f468be2f2a9f95c8f9f11534da7e8199488e9c Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 10 Nov 2020 13:35:11 +0800 Subject: [PATCH 01/25] Functionalize RandomKawaii --- website/src/views/components/RandomKawaii.tsx | 50 ++++--------------- 1 file changed, 11 insertions(+), 39 deletions(-) diff --git a/website/src/views/components/RandomKawaii.tsx b/website/src/views/components/RandomKawaii.tsx index 3e28704d51..e2c35f4c6c 100644 --- a/website/src/views/components/RandomKawaii.tsx +++ b/website/src/views/components/RandomKawaii.tsx @@ -1,48 +1,20 @@ -import * as React from 'react'; +import { FC, HTMLAttributes, useState } from 'react'; import { sample } from 'lodash'; import { SpeechBubble, Mug, Browser, Ghost, KawaiiMood, KawaiiProps } from 'react-kawaii'; -type Props = { - size?: number; - mood?: KawaiiMood; - color?: string; - title?: string; - 'aria-label'?: string; -}; +type Props = HTMLAttributes & KawaiiProps; const icons = [SpeechBubble, Mug, Browser, Ghost]; const defaultMoods: KawaiiMood[] = ['ko', 'sad', 'shocked']; -class RandomKawaii extends React.PureComponent { - kawaii: React.ComponentType; - - defaultMood: KawaiiMood; - - static defaultProps = { - mood: null, - color: '#FF715D', - }; - - constructor(props: Props) { - super(props); - - /* eslint-disable @typescript-eslint/no-non-null-assertion */ - this.kawaii = sample(icons)!; - this.defaultMood = sample(defaultMoods)!; - /* eslint-enable */ - } - - render() { - const { size, mood, color, ...wrapperProps } = this.props; - const Kawaii = this.kawaii; - const moodProp = mood || this.defaultMood; - - return ( -
- -
- ); - } -} +const RandomKawaii: FC = ({ size, color = '#FF715D', mood, ...wrapperProps }) => { + const [Kawaii] = useState(() => sample(icons) ?? icons[0]); + const [defaultMood] = useState(() => sample(defaultMoods) ?? defaultMoods[0]); + return ( +
+ +
+ ); +}; export default RandomKawaii; From 44cd5ce95b0e0763e9e42f5905a1d208ba899243 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 10 Nov 2020 14:26:31 +0800 Subject: [PATCH 02/25] Functionalize KeyboardShortcuts --- .../views/components/KeyboardShortcuts.tsx | 192 ++++++++---------- 1 file changed, 87 insertions(+), 105 deletions(-) diff --git a/website/src/views/components/KeyboardShortcuts.tsx b/website/src/views/components/KeyboardShortcuts.tsx index 34a7128791..6c18d50937 100644 --- a/website/src/views/components/KeyboardShortcuts.tsx +++ b/website/src/views/components/KeyboardShortcuts.tsx @@ -1,32 +1,20 @@ -import * as React from 'react'; -import { withRouter, RouteComponentProps } from 'react-router-dom'; -import { Dispatch } from 'redux'; -import { connect } from 'react-redux'; +import { memo, useCallback, useEffect, useRef, useState } from 'react'; +import { useHistory } from 'react-router-dom'; +import { useDispatch, useSelector } from 'react-redux'; import Mousetrap from 'mousetrap'; import { groupBy, map } from 'lodash'; -import { Mode, ThemeId, DARK_MODE } from 'types/settings'; -import { Actions } from 'types/actions'; +import { DARK_MODE } from 'types/settings'; import themes from 'data/themes.json'; import { cycleTheme, toggleTimetableOrientation } from 'actions/theme'; import { openNotification } from 'actions/app'; import { toggleMode } from 'actions/settings'; import { intersperse } from 'utils/array'; import ComponentMap from 'utils/ComponentMap'; -import { State as StoreState } from 'types/state'; +import type { State } from 'types/state'; import Modal from './Modal'; import styles from './KeyboardShortcuts.scss'; -type Props = RouteComponentProps & { - dispatch: Dispatch; - theme: ThemeId; - mode: Mode; -}; - -type State = { - helpShown: boolean; -}; - type Section = 'Appearance' | 'Navigation' | 'Timetable'; const APPEARANCE: Section = 'Appearance'; const NAVIGATION: Section = 'Navigation'; @@ -41,38 +29,52 @@ type KeyBinding = { const THEME_NOTIFICATION_TIMEOUT = 1000; -export class KeyboardShortcutsComponent extends React.PureComponent { - state = { - helpShown: false, - }; +const KeyboardShortcuts: React.FC = () => { + const [helpShown, setHelpShown] = useState(false); + const closeModal = useCallback(() => setHelpShown(false), []); - shortcuts: KeyBinding[] = []; + const mode = useSelector(({ settings }: State) => settings.mode); + const themeId = useSelector(({ theme }: State) => theme.id); + const dispatch = useDispatch(); - componentDidMount() { - const { dispatch, history } = this.props; + const history = useHistory(); + + // NB: Because this is a ref, updates to `shortcuts` will not trigger a render. + const shortcuts = useRef([]); + + useEffect(() => { + function bind( + key: Shortcut, + section: Section, + description: string, + action: (e: Event) => void, + ) { + shortcuts.current.push({ key, description, section }); + Mousetrap.bind(key, action); + } // Navigation - this.bind('y', NAVIGATION, 'Go to today', () => { + bind('y', NAVIGATION, 'Go to today', () => { history.push('/today'); }); - this.bind('t', NAVIGATION, 'Go to timetable', () => { + bind('t', NAVIGATION, 'Go to timetable', () => { history.push('/timetable'); }); - this.bind('m', NAVIGATION, 'Go to module finder', () => { + bind('m', NAVIGATION, 'Go to module finder', () => { history.push('/modules'); }); - this.bind('v', NAVIGATION, 'Go to venues page', () => { + bind('v', NAVIGATION, 'Go to venues page', () => { history.push('/venues'); }); - this.bind('s', NAVIGATION, 'Go to settings', () => { + bind('s', NAVIGATION, 'Go to settings', () => { history.push('/settings'); }); - this.bind('/', NAVIGATION, 'Open global search', (e) => { + bind('/', NAVIGATION, 'Open global search', (e) => { if (ComponentMap.globalSearchInput) { ComponentMap.globalSearchInput.focus(); @@ -81,16 +83,14 @@ export class KeyboardShortcutsComponent extends React.PureComponent - this.setState((state) => ({ helpShown: !state.helpShown })), - ); + bind('?', NAVIGATION, 'Show this help', () => setHelpShown(!helpShown)); // Timetable shortcuts - this.bind('o', TIMETABLE, 'Switch timetable orientation', () => { + bind('o', TIMETABLE, 'Switch timetable orientation', () => { dispatch(toggleTimetableOrientation()); }); - this.bind('d', TIMETABLE, 'Open download timetable menu', () => { + bind('d', TIMETABLE, 'Open download timetable menu', () => { const button = ComponentMap.downloadButton; if (button) { button.focus(); @@ -99,101 +99,83 @@ export class KeyboardShortcutsComponent extends React.PureComponent { - this.props.dispatch(toggleMode()); + bind('x', APPEARANCE, 'Toggle Night Mode', () => { + dispatch(toggleMode()); dispatch( - openNotification(`Night mode ${this.props.mode === DARK_MODE ? 'on' : 'off'}`, { + openNotification(`Night mode ${mode === DARK_MODE ? 'on' : 'off'}`, { overwritable: true, }), ); }); // Cycle through themes - this.bind('z', APPEARANCE, 'Previous Theme', () => { + function notifyThemeChange() { + const theme = themes.find((t) => t.id === themeId); + if (theme) { + dispatch( + openNotification(`Theme switched to ${theme.name}`, { + timeout: THEME_NOTIFICATION_TIMEOUT, + overwritable: true, + }), + ); + } + } + + bind('z', APPEARANCE, 'Previous Theme', () => { dispatch(cycleTheme(-1)); - this.notifyThemeChange(); + notifyThemeChange(); }); - this.bind('c', APPEARANCE, 'Next Theme', () => { + bind(['c', '0'], APPEARANCE, 'Next Theme', () => { dispatch(cycleTheme(1)); - this.notifyThemeChange(); + notifyThemeChange(); }); // ??? Mousetrap.bind('up up down down left right left right b a', () => { history.push('/tetris'); }); - } - closeModal = () => this.setState({ helpShown: false }); - - bind(key: Shortcut, section: Section, description: string, action: (e: Event) => void) { - this.shortcuts.push({ key, description, section }); - - Mousetrap.bind(key, action); - } + return () => { + shortcuts.current.forEach(({ key }) => Mousetrap.unbind(key)); + shortcuts.current = []; + }; + }, [dispatch, helpShown, mode, history, themeId]); - notifyThemeChange() { - const themeId = this.props.theme; - const theme = themes.find((t) => t.id === themeId); - - if (theme) { - this.props.dispatch( - openNotification(`Theme switched to ${theme.name}`, { - timeout: THEME_NOTIFICATION_TIMEOUT, - overwritable: true, - }), - ); - } - } - - renderShortcut = (shortcut: Shortcut): React.ReactNode => { + function renderShortcut(shortcut: Shortcut): React.ReactNode { if (typeof shortcut === 'string') { const capitalized = shortcut.replace(/\b([a-z])/, (c) => c.toUpperCase()); return {capitalized}; } + return intersperse(shortcut.map(renderShortcut), ' or '); + } - return intersperse(shortcut.map(this.renderShortcut), ' or '); - }; - - render() { - const sections = groupBy(this.shortcuts, (shortcut) => shortcut.section); - - return ( - -

Keyboard shortcuts

- - - {map(sections, (shortcuts, heading) => ( - - - - + const sections = groupBy(shortcuts.current, (shortcut) => shortcut.section); - {shortcuts.map((shortcut) => ( - - - - - ))} - - ))} -
- {heading}
{this.renderShortcut(shortcut.key)}{shortcut.description}
- - ); - } -} + return ( + +

Keyboard shortcuts

-const KeyboardShortcutsConnected = connect((state: StoreState) => ({ - mode: state.settings.mode, - theme: state.theme.id, -}))(KeyboardShortcutsComponent); + + {map(sections, (shortcutsInSection, heading) => ( + + + + + + {shortcutsInSection.map((shortcut) => ( + + + + + ))} + + ))} +
+ {heading}
{renderShortcut(shortcut.key)}{shortcut.description}
+ + ); +}; -export default withRouter(KeyboardShortcutsConnected); +export default memo(KeyboardShortcuts); From e125590bff890c867a7115cf02c37a8d0ff1b168 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 10 Nov 2020 14:29:40 +0800 Subject: [PATCH 03/25] Functionalize ScrollToTop --- .../src/views/components/ScrollToTop.test.tsx | 49 ++++++------ website/src/views/components/ScrollToTop.tsx | 75 ++++++++++--------- .../src/views/modules/ModulePageContent.tsx | 2 +- 3 files changed, 63 insertions(+), 63 deletions(-) diff --git a/website/src/views/components/ScrollToTop.test.tsx b/website/src/views/components/ScrollToTop.test.tsx index 40db9094df..c4c8367fb8 100644 --- a/website/src/views/components/ScrollToTop.test.tsx +++ b/website/src/views/components/ScrollToTop.test.tsx @@ -1,8 +1,10 @@ -import { MemoryRouter } from 'react-router-dom'; -import { mount, ReactWrapper } from 'enzyme'; +import { act } from 'react-dom/test-utils'; +import { Router } from 'react-router-dom'; +import { mount } from 'enzyme'; +import createHistory from 'test-utils/createHistory'; import mockDom from 'test-utils/mockDom'; -import ScrollToTop, { ScrollToTopComponent } from './ScrollToTop'; +import ScrollToTop from './ScrollToTop'; type Props = { onComponentDidMount?: boolean; @@ -14,21 +16,19 @@ describe('ScrollToTopComponent', () => { mockDom(); }); - // Construct a testable ScrollToTop component function make(props: Props = {}) { - return mount( - - {} - - , - ); - } - - function getHistory(wrapper: ReactWrapper) { - return wrapper.find(ScrollToTopComponent).prop('history'); + const { history } = createHistory(); + act(() => { + mount( + + + , + ); + }); + return history; } test('default behavior does not do anything', () => { @@ -42,10 +42,9 @@ describe('ScrollToTopComponent', () => { }); test('onPathChange attribute behaves correctly', () => { - const wrapper = make({ onPathChange: true }); - const history = getHistory(wrapper); + const history = make({ onPathChange: true }); expect(window.scrollTo).not.toHaveBeenCalled(); - history.push('/foo'); + act(() => history.push('/foo')); expect(window.scrollTo).toHaveBeenCalledWith(0, 0); }); @@ -55,17 +54,15 @@ describe('ScrollToTopComponent', () => { }); test('integration test', () => { - const wrapper = make({ onComponentDidMount: true, onPathChange: true }); - const history = getHistory(wrapper); - + const history = make({ onComponentDidMount: true, onPathChange: true }); expect(window.scrollTo).toHaveBeenCalledTimes(1); expect(window.scrollTo).toHaveBeenCalledWith(0, 0); - history.push('/foo'); + act(() => history.push('/foo')); expect(window.scrollTo).toHaveBeenCalledTimes(2); expect(window.scrollTo).toHaveBeenCalledWith(0, 0); - history.push('/foo'); + act(() => history.push('/foo')); expect(window.scrollTo).toHaveBeenCalledTimes(2); - history.push('/bar'); + act(() => history.push('/bar')); expect(window.scrollTo).toHaveBeenCalledTimes(3); }); }); diff --git a/website/src/views/components/ScrollToTop.tsx b/website/src/views/components/ScrollToTop.tsx index c9c49b9313..710c4fb3e8 100644 --- a/website/src/views/components/ScrollToTop.tsx +++ b/website/src/views/components/ScrollToTop.tsx @@ -1,50 +1,53 @@ -import { Component } from 'react'; -import { withRouter, RouteComponentProps } from 'react-router-dom'; +import { useEffect, useRef } from 'react'; +import type { LocationKey } from 'history'; +import { useLocation } from 'react-router-dom'; import { scrollToHash } from 'utils/react'; -export type Props = RouteComponentProps & { - onComponentDidMount?: boolean; - onPathChange?: boolean; - scrollToHash?: boolean; -}; - function scrollToTop() { window.scrollTo(0, 0); } -export class ScrollToTopComponent extends Component { - static defaultProps = { - onComponentDidMount: false, - onPathChange: false, - scrollToHash: true, - }; - - componentDidMount() { - if (this.props.onComponentDidMount && !window.location.hash) { - scrollToTop(); - } else if (this.props.scrollToHash) { - scrollToHash(); - } - } - - componentDidUpdate(prevProps: Props) { - const { - onPathChange, - location: { pathname, hash }, - } = this.props; +export type Props = { + onComponentDidMount?: boolean; + onPathChange?: boolean; + shouldScrollToHash?: boolean; +}; +const ScrollToTop: React.FC = ({ + onComponentDidMount = false, + onPathChange = false, + shouldScrollToHash = true, +}) => { + useEffect( + () => { + if (onComponentDidMount && !window.location.hash) { + scrollToTop(); + } else if (shouldScrollToHash) { + scrollToHash(); + } + }, + // This effect should only be run on component mount; don't care if props + // change afterwards. + // eslint-disable-next-line react-hooks/exhaustive-deps + [], + ); + + const previousLocationKey = useRef(null); + const location = useLocation(); + useEffect(() => { if ( onPathChange && - pathname !== prevProps.location.pathname && - hash === prevProps.location.hash + // Don't scroll to top on initial mount (i.e. when previousLocationKey.current is null) + previousLocationKey.current && + // Don't scroll to top if no navigation has happened + previousLocationKey.current !== location.pathname ) { scrollToTop(); } - } + previousLocationKey.current = location.pathname; + }, [onPathChange, location.pathname]); - render() { - return null; - } -} + return null; +}; -export default withRouter(ScrollToTopComponent); +export default ScrollToTop; diff --git a/website/src/views/modules/ModulePageContent.tsx b/website/src/views/modules/ModulePageContent.tsx index d1455afeb9..e8939b259d 100644 --- a/website/src/views/modules/ModulePageContent.tsx +++ b/website/src/views/modules/ModulePageContent.tsx @@ -81,7 +81,7 @@ class ModulePageContent extends Component { - + {isArchive && (
From 93bcb64a2646fcd60b8a8e26eb97bdd7174d2bcf Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Mon, 30 Nov 2020 16:20:42 +0800 Subject: [PATCH 04/25] Functionalize AppShell --- website/src/views/AppShell.tsx | 290 ++++++++++++++++----------------- 1 file changed, 140 insertions(+), 150 deletions(-) diff --git a/website/src/views/AppShell.tsx b/website/src/views/AppShell.tsx index 7dd461e1fb..2fdecff908 100644 --- a/website/src/views/AppShell.tsx +++ b/website/src/views/AppShell.tsx @@ -1,20 +1,22 @@ -import * as React from 'react'; -import { SemTimetableConfig, TimetableConfig } from 'types/timetables'; -import { ModuleList, NotificationOptions } from 'types/reducers'; -import { Semester } from 'types/modules'; -import { DARK_MODE, Mode } from 'types/settings'; +import { FC, useCallback, useEffect, useState } from 'react'; +import type { SemTimetableConfig } from 'types/timetables'; +import type { Semester } from 'types/modules'; +import { DARK_MODE } from 'types/settings'; import { Helmet } from 'react-helmet'; -import { NavLink, RouteComponentProps, withRouter } from 'react-router-dom'; -import { connect } from 'react-redux'; +import { NavLink, useHistory } from 'react-router-dom'; +import { useDispatch, useSelector, useStore } from 'react-redux'; import classnames from 'classnames'; import { each } from 'lodash'; import weekText from 'utils/weekText'; import { captureException } from 'utils/error'; import { openNotification } from 'actions/app'; -import { fetchModuleList } from 'actions/moduleBank'; -import { fetchTimetableModules, setTimetable, validateTimetable } from 'actions/timetables'; +import { fetchModuleList as fetchModuleListAction } from 'actions/moduleBank'; +import { + fetchTimetableModules as fetchTimetableModulesAction, + validateTimetable, +} from 'actions/timetables'; import Footer from 'views/layout/Footer'; import Navtabs from 'views/layout/Navtabs'; import GlobalSearchContainer from 'views/layout/GlobalSearchContainer'; @@ -25,172 +27,160 @@ import ApiError from 'views/errors/ApiError'; import { trackPageView } from 'bootstrapping/matomo'; import { isIOS } from 'bootstrapping/browser'; import Logo from 'img/nusmods-logo.svg'; -import { State as StoreState } from 'types/state'; +import type { Dispatch } from 'types/redux'; +import type { State } from 'types/state'; +import type { Actions } from 'types/actions'; import LoadingSpinner from './components/LoadingSpinner'; import FeedbackModal from './components/FeedbackModal'; import KeyboardShortcuts from './components/KeyboardShortcuts'; import styles from './AppShell.scss'; -type Props = RouteComponentProps & { - children: React.ReactNode; - - // From Redux state - moduleList: ModuleList; - timetables: TimetableConfig; - theme: string; - mode: Mode; - activeSemester: Semester; - - // From Redux actions - fetchModuleList: () => Promise; // Typed as unknown because we don't actually need the output - fetchTimetableModules: (semTimetableConfig: SemTimetableConfig[]) => Promise; - setTimetable: (semester: Semester, semTimetableConfig: SemTimetableConfig) => void; - validateTimetable: (semester: Semester) => void; - openNotification: (str: string, notificationOptions?: NotificationOptions) => void; -}; - -type State = { - moduleListError?: Error; -}; - -export class AppShellComponent extends React.Component { - state: State = {}; - - componentDidMount() { - const { timetables } = this.props; +/** + * Fetch module list on mount. + */ +function useFetchModuleListAndTimetableModules(): { + moduleListError: Error | null; + refetchModuleListAndTimetableModules: () => void; +} { + const [moduleListError, setModuleListError] = useState(null); + + const dispatch = useDispatch(); + const store = useStore(); + + const fetchModuleList = useCallback( + () => + // TODO: This always re-fetch the entire modules list. Consider a better strategy for this + dispatch(fetchModuleListAction()).catch((error) => { + captureException(error); + setModuleListError(error); + }), + [dispatch], + ); + + const fetchTimetableModules = useCallback( + function fetchTimetableModulesImpl(timetable: SemTimetableConfig, semester: Semester) { + dispatch(fetchTimetableModulesAction([timetable])) + .then(() => dispatch(validateTimetable(semester))) + .catch((error) => { + captureException(error); + dispatch( + openNotification('Data for some modules failed to load', { + action: { + text: 'Retry', + handler: () => fetchTimetableModulesImpl(timetable, semester), + }, + }), + ); + }); + }, + [dispatch], + ); + const fetchModuleListAndTimetableModules = useCallback(() => { // Retrieve module list - const moduleList = this.fetchModuleList(); + const moduleListPromise = fetchModuleList(); // Fetch the module data of the existing modules in the timetable and ensure all // lessons are filled + const timetables = store.getState().timetables.lessons; each(timetables, (timetable, semesterString) => { const semester = Number(semesterString); - moduleList.then(() => { + moduleListPromise.then(() => { // Wait for module list to be fetched before trying to fetch timetable modules // TODO: There may be a more optimal way to do this - this.fetchTimetableModules(timetable, semester); + fetchTimetableModules(timetable, semester); }); }); + }, [fetchModuleList, fetchTimetableModules, store]); - // Enable Matomo analytics - trackPageView(this.props.history); - } + useEffect(() => fetchModuleListAndTimetableModules(), [fetchModuleListAndTimetableModules]); - fetchModuleList = () => - // TODO: This always re-fetch the entire modules list. Consider a better strategy for this - this.props.fetchModuleList().catch((error) => { - captureException(error); - this.setState({ moduleListError: error }); - }); - - fetchTimetableModules = (timetable: SemTimetableConfig, semester: Semester) => { - this.props - .fetchTimetableModules([timetable]) - .then(() => this.props.validateTimetable(semester)) - .catch((error) => { - captureException(error); - this.props.openNotification('Data for some modules failed to load', { - action: { - text: 'Retry', - handler: () => this.fetchTimetableModules(timetable, semester), - }, - }); - }); + return { + moduleListError, + refetchModuleListAndTimetableModules: fetchModuleListAndTimetableModules, }; +} - render() { - const isModuleListReady = this.props.moduleList.length; - const isDarkMode = this.props.mode === DARK_MODE; - - if (!isModuleListReady && this.state.moduleListError) { - return ; - } - - return ( -
- - - - - - -
- - -
- {isModuleListReady ? ( - }> - {this.props.children} - - ) : ( - - )} -
-
+ // Enable Matomo analytics + const history = useHistory(); + useEffect(() => trackPageView(history), [history]); - - - + const moduleList = useSelector((state: State) => state.moduleBank.moduleList); + const isModuleListReady = moduleList.length; - - - + const mode = useSelector((state: State) => state.settings.mode); + const isDarkMode = mode === DARK_MODE; - -
- + const theme = useSelector((state: State) => state.theme.id); - - - -
- ); + if (!isModuleListReady && moduleListError) { + return ; } -} -const mapStateToProps = (state: StoreState) => ({ - moduleList: state.moduleBank.moduleList, - timetables: state.timetables.lessons, - theme: state.theme.id, - mode: state.settings.mode, - activeSemester: state.app.activeSemester, -}); - -const connectedAppShell = connect( - mapStateToProps, - { - fetchModuleList, - fetchTimetableModules, - setTimetable, - validateTimetable, - openNotification, - }, - // TODO: Patch types for Redux for request-middleware - // eslint-disable-next-line @typescript-eslint/no-explicit-any -)(AppShellComponent as any); - -// withRouter here is used to ensure re-render when routes change, since -// connect implements shouldComponentUpdate based purely on props. If it -// is removed, connect not detect prop changes when route is changed and -// thus the pages are not re-rendered -export default withRouter(connectedAppShell); + return ( +
+ + + + + + +
+ + +
+ {isModuleListReady ? ( + }> + {children} + + ) : ( + + )} +
+
+ + + + + + + + + + +
+ + + + + +
+ ); +}; + +export default AppShell; From 6573396358d950dcb9a3f31589ca2859331dd151 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Mon, 30 Nov 2020 16:38:06 +0800 Subject: [PATCH 05/25] Functionalize LessonTimetable --- .../module-info/LessonTimetable.tsx | 99 ++++++------- .../components/module-info/SemesterPicker.tsx | 134 +++++++++--------- .../src/views/modules/ModulePageContent.tsx | 2 +- 3 files changed, 107 insertions(+), 128 deletions(-) diff --git a/website/src/views/components/module-info/LessonTimetable.tsx b/website/src/views/components/module-info/LessonTimetable.tsx index 024c98ef17..bac2eb7b4e 100644 --- a/website/src/views/components/module-info/LessonTimetable.tsx +++ b/website/src/views/components/module-info/LessonTimetable.tsx @@ -1,7 +1,8 @@ -import * as React from 'react'; -import { RouteComponentProps, withRouter } from 'react-router-dom'; +import { FC, useState } from 'react'; +import { useHistory } from 'react-router-dom'; -import { Semester, SemesterData } from 'types/modules'; +import type { SemesterData } from 'types/modules'; +import type { Lesson } from 'types/timetables'; import Timetable from 'views/timetable/Timetable'; import SemesterPicker from 'views/components/module-info/SemesterPicker'; @@ -9,68 +10,48 @@ import { arrangeLessonsForWeek } from 'utils/timetables'; import { colorLessonsByKey } from 'utils/colors'; import { getFirstAvailableSemester } from 'utils/modules'; import { venuePage } from 'views/routes/paths'; -import { Lesson } from 'types/timetables'; import styles from './LessonTimetable.scss'; -type Props = RouteComponentProps & { semesterData: readonly SemesterData[] }; +const SemesterLessonTimetable: FC<{ semesterData?: SemesterData }> = ({ semesterData }) => { + const history = useHistory(); -type State = { - selectedSem: Semester; -}; - -export class LessonTimetableComponent extends React.PureComponent { - constructor(props: Props) { - super(props); - this.state = { - selectedSem: getFirstAvailableSemester(props.semesterData), - }; + if (!semesterData?.timetable) { + return

Timetable info not available

; } - onSelectSemester = (selectedSem: Semester) => { - this.setState({ selectedSem }); - }; - - renderTimetable(): React.ReactNode { - const semester = this.props.semesterData.find( - (data) => data.semester === this.state.selectedSem, - ); - if (!semester || !semester.timetable) { - return

Timetable info not available

; - } - - const lessons = semester.timetable.map((lesson) => ({ - ...lesson, - moduleCode: '', - title: '', - isModifiable: !!lesson.venue, - })); - const coloredLessons = colorLessonsByKey(lessons, 'lessonType'); - const arrangedLessons = arrangeLessonsForWeek(coloredLessons); + const lessons = semesterData.timetable.map((lesson) => ({ + ...lesson, + moduleCode: '', + title: '', + isModifiable: !!lesson.venue, + })); + const coloredLessons = colorLessonsByKey(lessons, 'lessonType'); + const arrangedLessons = arrangeLessonsForWeek(coloredLessons); + + return ( + history.push(venuePage(lesson.venue))} + /> + ); +}; - return ( - { - this.props.history.push(venuePage(lesson.venue)); - }} +const LessonTimetable: FC<{ allSemesterData: SemesterData[] }> = ({ allSemesterData }) => { + const [selectedSem, setSelectedSem] = useState(() => getFirstAvailableSemester(allSemesterData)); + return ( + <> + data.semester)} + selectedSemester={selectedSem} + onSelectSemester={setSelectedSem} /> - ); - } - - render() { - const semesters = this.props.semesterData.map((data) => data.semester); - - return ( - <> - + data.semester === selectedSem)} /> -
{this.renderTimetable()}
- - ); - } -} +
+ + ); +}; -export default withRouter(LessonTimetableComponent); +export default LessonTimetable; diff --git a/website/src/views/components/module-info/SemesterPicker.tsx b/website/src/views/components/module-info/SemesterPicker.tsx index edaccdecca..e625c1c85c 100644 --- a/website/src/views/components/module-info/SemesterPicker.tsx +++ b/website/src/views/components/module-info/SemesterPicker.tsx @@ -1,7 +1,7 @@ -import { memo } from 'react'; +import { FC, memo } from 'react'; import { each } from 'lodash'; -import { Semester } from 'types/modules'; +import type { Semester } from 'types/modules'; import ButtonGroupSelector, { Props as ButtonGroupProps, @@ -18,71 +18,69 @@ type Props = { onSelectSemester: (semester: Semester) => void; }; -const SemesterPicker = memo( - ({ - semesters, - showDisabled = false, - useShortNames = false, - size, - selectedSemester, - onSelectSemester, - }) => { - const semesterNames = () => { - return useShortNames ? config.shortSemesterNames : config.semesterNames; - }; - - /** - * Map button labels (semester names) to semesters - */ - const semesterMap = (): { [key: string]: Semester | null } => { - const map: { [key: string]: Semester | null } = {}; - - each(semesterNames(), (name: string, key: string) => { - const semester = semesters.find((sem) => String(sem) === key); - if (semester || showDisabled) map[name] = semester || null; - }); - - return map; - }; - - const onSelectSemesterInner = (choice: string) => { - const chosen = semesterMap()[choice]; - - if (chosen) { - onSelectSemester(Number(chosen)); +const SemesterPicker: FC = ({ + semesters, + showDisabled = false, + useShortNames = false, + size, + selectedSemester, + onSelectSemester, +}) => { + const semesterNames = () => { + return useShortNames ? config.shortSemesterNames : config.semesterNames; + }; + + /** + * Map button labels (semester names) to semesters + */ + const semesterMap = (): { [key: string]: Semester | null } => { + const map: { [key: string]: Semester | null } = {}; + + each(semesterNames(), (name: string, key: string) => { + const semester = semesters.find((sem) => String(sem) === key); + if (semester || showDisabled) map[name] = semester || null; + }); + + return map; + }; + + const onSelectSemesterInner = (choice: string) => { + const chosen = semesterMap()[choice]; + + if (chosen) { + onSelectSemester(Number(chosen)); + } + }; + + // Disable and add title for buttons representing semesters that are not available + const buttonAttrs = () => { + const semesterMapObtained = semesterMap(); + const attrs: ButtonGroupProps['attrs'] = {}; + + each(semesterNames(), (name: string) => { + if (!semesterMapObtained[name]) { + attrs[name] = { + disabled: true, + title: `This module is not available in ${name}`, + }; } - }; - - // Disable and add title for buttons representing semesters that are not available - const buttonAttrs = () => { - const semesterMapObtained = semesterMap(); - const attrs: ButtonGroupProps['attrs'] = {}; - - each(semesterNames(), (name: string) => { - if (!semesterMapObtained[name]) { - attrs[name] = { - disabled: true, - title: `This module is not available in ${name}`, - }; - } - }); - - return { attrs }; - }; + }); + + return { attrs }; + }; + + const semesterMapObtained = semesterMap(); + const selected = selectedSemester ? semesterNames()[selectedSemester] : null; + + return ( + + ); +}; - const semesterMapObtained = semesterMap(); - const selected = selectedSemester ? semesterNames()[selectedSemester] : null; - - return ( - - ); - }, -); - -export default SemesterPicker; +export default memo(SemesterPicker); diff --git a/website/src/views/modules/ModulePageContent.tsx b/website/src/views/modules/ModulePageContent.tsx index e8939b259d..38c37ac123 100644 --- a/website/src/views/modules/ModulePageContent.tsx +++ b/website/src/views/modules/ModulePageContent.tsx @@ -240,7 +240,7 @@ class ModulePageContent extends Component {

Timetable

- +
From f5ccc16e6dad2510e645cd56c13cfbd8a0c65747 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Mon, 30 Nov 2020 16:51:49 +0800 Subject: [PATCH 06/25] Functionalize ModRegNotification --- .../notfications/ModRegNotification.tsx | 92 +++++++++---------- 1 file changed, 43 insertions(+), 49 deletions(-) diff --git a/website/src/views/components/notfications/ModRegNotification.tsx b/website/src/views/components/notfications/ModRegNotification.tsx index a64e469e82..281af348dc 100644 --- a/website/src/views/components/notfications/ModRegNotification.tsx +++ b/website/src/views/components/notfications/ModRegNotification.tsx @@ -1,10 +1,9 @@ -import * as React from 'react'; -import { connect } from 'react-redux'; -import { withRouter, RouteComponentProps } from 'react-router-dom'; +import { FC, memo, useCallback } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { useHistory } from 'react-router-dom'; import { formatDistance } from 'date-fns'; -import { NotificationOptions } from 'types/reducers'; -import { State } from 'types/state'; +import type { State } from 'types/state'; import config, { RegPeriod } from 'config'; import { dismissModregNotification } from 'actions/settings'; @@ -16,25 +15,15 @@ import { forceTimer } from 'utils/debug'; import styles from './ModRegNotification.scss'; -type Props = { - // True only on the settings page since we don't want - // users accidentally dismissing the preview notification - dismissible?: boolean; - rounds: RegPeriod[]; - - dismissModregNotification: (round: RegPeriod) => void; - openNotification: (str: string, notificationOptions: NotificationOptions) => void; -} & RouteComponentProps; - // Holy shit const MOD_REG_URL = 'https://myedurec.nus.edu.sg/psc/cs90prd/EMPLOYEE/SA/c/N_STUDENT_RECORDS.N_MRS_START_MD_FL.GBL?Action=U&MD=Y&GMenu=N_STUDENT_RECORDS&GComp=N_MRS_START_NAV_FL&GPage=N_MRS_START_NAV_FL&scname=N_MRS_MODULE_REG_NAV&dup=Y&'; -export function notificationText( - useLineBreaks: boolean, - round: RegPeriod, - now: Date, -): React.ReactNode { +const NotificationText: FC<{ + useLineBreaks: boolean; + round: RegPeriod; + now: Date; +}> = ({ useLineBreaks, round, now }) => { const isRoundOpen = now >= round.startDate; const timeFromNow = formatDistance(now, isRoundOpen ? round.endDate : round.startDate, { includeSeconds: true, @@ -48,23 +37,40 @@ export function notificationText( {useLineBreaks &&
} ({timeFromNow} from now) ); -} +}; -export const ModRegNotificationComponent: React.FC = (props) => { - const dismiss = (round: RegPeriod) => { - props.dismissModregNotification(round); - props.openNotification('Reminder snoozed until start of next round', { - timeout: 12000, - action: { - text: 'Settings', - handler: () => { - props.history.push('/settings#modreg'); - }, - }, - }); - }; +const ModRegNotification: FC<{ + // True only on the settings page since we don't want + // users accidentally dismissing the preview notification + dismissible?: boolean; +}> = ({ dismissible }) => { + const rounds = useSelector(({ settings }: State) => + getRounds(settings.modRegNotification, config.modRegSchedule).filter( + (round) => !round.dismissed, + ), + ); + const dispatch = useDispatch(); + + const history = useHistory(); + + const dismiss = useCallback( + (round: RegPeriod) => { + dispatch(dismissModregNotification(round)); + dispatch( + openNotification('Reminder snoozed until start of next round', { + timeout: 12000, + action: { + text: 'Settings', + handler: () => { + history.push('/settings#modreg'); + }, + }, + }), + ); + }, + [dispatch, history], + ); - const { rounds, dismissible } = props; if (!rounds.length) return null; return ( @@ -73,7 +79,7 @@ export const ModRegNotificationComponent: React.FC = (props) => {
- {notificationText(true, round, forceTimer() || new Date())} +
@@ -84,16 +90,4 @@ export const ModRegNotificationComponent: React.FC = (props) => { ); }; -const mapStateToProps = (state: State) => ({ - rounds: getRounds(state.settings.modRegNotification, config.modRegSchedule).filter( - (round) => !round.dismissed, - ), -}); - -const withStoreModRegNotification = connect(mapStateToProps, { - dismissModregNotification, - openNotification, -})(React.memo(ModRegNotificationComponent)); - -const ModRegNotification = withRouter(withStoreModRegNotification); -export default ModRegNotification; +export default memo(ModRegNotification); From 4559d71256329c4cdc9d8f56a55d3528b5bf29f7 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Mon, 30 Nov 2020 18:21:03 +0800 Subject: [PATCH 07/25] Add React Testing Library For a more modern testing API that: 1. Discourages testing implementation details. 2. Makes it easier to test user-visible behavior. --- website/jest.common.config.js | 2 +- website/package.json | 3 + website/scripts/test.js | 1 + website/yarn.lock | 108 +++++++++++++++++++++++++++++++++- 4 files changed, 110 insertions(+), 4 deletions(-) diff --git a/website/jest.common.config.js b/website/jest.common.config.js index 655cb8c686..1f4d8bd6d1 100644 --- a/website/jest.common.config.js +++ b/website/jest.common.config.js @@ -2,7 +2,7 @@ module.exports = { roots: ['/src'], moduleDirectories: ['node_modules', '/src'], moduleFileExtensions: ['jsx', 'js', 'ts', 'tsx'], - setupFiles: ['/scripts/test.js'], + setupFilesAfterEnv: ['/scripts/test.js'], moduleNameMapper: { // Mock non JS files as strings '\\.(?:jpg|jpeg|png|gif|eot|otf|webp|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$': diff --git a/website/package.json b/website/package.json index 9c4796afa1..a985869274 100644 --- a/website/package.json +++ b/website/package.json @@ -37,6 +37,9 @@ "@packtracker/webpack-plugin": "2.3.0", "@pmmmwh/react-refresh-webpack-plugin": "0.4.3", "@svgr/webpack": "5.5.0", + "@testing-library/jest-dom": "^5.11.6", + "@testing-library/react": "^11.2.2", + "@testing-library/user-event": "^12.2.2", "@types/classnames": "2.2.11", "@types/enzyme": "3.10.8", "@types/jest": "26.0.15", diff --git a/website/scripts/test.js b/website/scripts/test.js index 4244857212..1e986fbf84 100644 --- a/website/scripts/test.js +++ b/website/scripts/test.js @@ -1,6 +1,7 @@ const { configure } = require('enzyme'); const { setAutoFreeze } = require('immer'); const Adapter = require('@wojtekmaj/enzyme-adapter-react-17'); +require('@testing-library/jest-dom'); configure({ adapter: new Adapter() }); diff --git a/website/yarn.lock b/website/yarn.lock index 965556b90e..bd1cb6faab 100644 --- a/website/yarn.lock +++ b/website/yarn.lock @@ -1663,6 +1663,13 @@ dependencies: regenerator-runtime "^0.13.4" +"@babel/runtime@^7.12.5", "@babel/runtime@^7.9.2": + version "7.12.5" + resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.12.5.tgz#410e7e487441e1b360c29be715d870d9b985882e" + integrity sha512-plcc+hbExy3McchJCEQG3knOsuh3HH+Prx1P6cLIkET/0dLuQDEnrT+s27Axgc9bqfsmNUNHfscgMUdBpC9xfg== + dependencies: + regenerator-runtime "^0.13.4" + "@babel/runtime@^7.5.5": version "7.5.5" resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.5.5.tgz#74fba56d35efbeca444091c7850ccd494fd2f132" @@ -2513,6 +2520,49 @@ "@svgr/plugin-svgo" "^5.5.0" loader-utils "^2.0.0" +"@testing-library/dom@^7.28.1": + version "7.28.1" + resolved "https://registry.yarnpkg.com/@testing-library/dom/-/dom-7.28.1.tgz#dea78be6e1e6db32ddcb29a449e94d9700c79eb9" + integrity sha512-acv3l6kDwZkQif/YqJjstT3ks5aaI33uxGNVIQmdKzbZ2eMKgg3EV2tB84GDdc72k3Kjhl6mO8yUt6StVIdRDg== + dependencies: + "@babel/code-frame" "^7.10.4" + "@babel/runtime" "^7.12.5" + "@types/aria-query" "^4.2.0" + aria-query "^4.2.2" + chalk "^4.1.0" + dom-accessibility-api "^0.5.4" + lz-string "^1.4.4" + pretty-format "^26.6.2" + +"@testing-library/jest-dom@^5.11.6": + version "5.11.6" + resolved "https://registry.yarnpkg.com/@testing-library/jest-dom/-/jest-dom-5.11.6.tgz#782940e82e5cd17bc0a36f15156ba16f3570ac81" + integrity sha512-cVZyUNRWwUKI0++yepYpYX7uhrP398I+tGz4zOlLVlUYnZS+Svuxv4fwLeCIy7TnBYKXUaOlQr3vopxL8ZfEnA== + dependencies: + "@babel/runtime" "^7.9.2" + "@types/testing-library__jest-dom" "^5.9.1" + aria-query "^4.2.2" + chalk "^3.0.0" + css "^3.0.0" + css.escape "^1.5.1" + lodash "^4.17.15" + redent "^3.0.0" + +"@testing-library/react@^11.2.2": + version "11.2.2" + resolved "https://registry.yarnpkg.com/@testing-library/react/-/react-11.2.2.tgz#099c6c195140ff069211143cb31c0f8337bdb7b7" + integrity sha512-jaxm0hwUjv+hzC+UFEywic7buDC9JQ1q3cDsrWVSDAPmLotfA6E6kUHlYm/zOeGCac6g48DR36tFHxl7Zb+N5A== + dependencies: + "@babel/runtime" "^7.12.5" + "@testing-library/dom" "^7.28.1" + +"@testing-library/user-event@^12.2.2": + version "12.2.2" + resolved "https://registry.yarnpkg.com/@testing-library/user-event/-/user-event-12.2.2.tgz#22d0047da745289335240f523dfe74c889ec96cb" + integrity sha512-mTYL9LrwiSeyorStUOMuRGQDn1ca40tIhuv//o/K3lY8wBEp+9Im90MFVx5i3u7zCPmavn3uWZs/10chsbI8Tg== + dependencies: + "@babel/runtime" "^7.10.2" + "@tippy.js/react@3.1.1": version "3.1.1" resolved "https://registry.yarnpkg.com/@tippy.js/react/-/react-3.1.1.tgz#027e4595e55f31430741fe8e0d92aaddfbe47efd" @@ -2526,6 +2576,11 @@ resolved "https://registry.yarnpkg.com/@types/anymatch/-/anymatch-1.3.1.tgz#336badc1beecb9dacc38bea2cf32adf627a8421a" integrity sha512-/+CRPXpBDpo2RK9C68N3b2cOvO0Cf5B9aPijHsoDQTHivnGSObdOF2BRQOYjojWTDy6nQvMjmqRXIxH55VjxxA== +"@types/aria-query@^4.2.0": + version "4.2.0" + resolved "https://registry.yarnpkg.com/@types/aria-query/-/aria-query-4.2.0.tgz#14264692a9d6e2fa4db3df5e56e94b5e25647ac0" + integrity sha512-iIgQNzCm0v7QMhhe4Jjn9uRh+I6GoPmt03CbEtwx3ao8/EfoQcmgtqH4vQ5Db/lxiIGaWDv6nwvunuh0RyX0+A== + "@types/babel__core@^7.0.0": version "7.1.9" resolved "https://registry.yarnpkg.com/@types/babel__core/-/babel__core-7.1.9.tgz#77e59d438522a6fb898fa43dc3455c6e72f3963d" @@ -2696,7 +2751,7 @@ dependencies: "@types/istanbul-lib-report" "*" -"@types/jest@26.0.15": +"@types/jest@*", "@types/jest@26.0.15": version "26.0.15" resolved "https://registry.yarnpkg.com/@types/jest/-/jest-26.0.15.tgz#12e02c0372ad0548e07b9f4e19132b834cb1effe" integrity sha512-s2VMReFXRg9XXxV+CW9e5Nz8fH2K1aEhwgjUqPPbQd7g95T0laAcvLv032EhFHIa5GHsZ8W7iJEQVaJq6k3Gog== @@ -2928,6 +2983,13 @@ resolved "https://registry.yarnpkg.com/@types/tapable/-/tapable-1.0.5.tgz#9adbc12950582aa65ead76bffdf39fe0c27a3c02" integrity sha512-/gG2M/Imw7cQFp8PGvz/SwocNrmKFjFsm5Pb8HdbHkZ1K8pmuPzOX4VeVoiEecFCVf4CsN1r3/BRvx+6sNqwtQ== +"@types/testing-library__jest-dom@^5.9.1": + version "5.9.5" + resolved "https://registry.yarnpkg.com/@types/testing-library__jest-dom/-/testing-library__jest-dom-5.9.5.tgz#5bf25c91ad2d7b38f264b12275e5c92a66d849b0" + integrity sha512-ggn3ws+yRbOHog9GxnXiEZ/35Mow6YtPZpd7Z5mKDeZS/o7zx3yAle0ov/wjhVB5QT4N2Dt+GNoGCdqkBGCajQ== + dependencies: + "@types/jest" "*" + "@types/uglify-js@*": version "3.0.4" resolved "https://registry.yarnpkg.com/@types/uglify-js/-/uglify-js-3.0.4.tgz#96beae23df6f561862a830b4288a49e86baac082" @@ -3772,7 +3834,7 @@ at-least-node@^1.0.0: resolved "https://registry.yarnpkg.com/at-least-node/-/at-least-node-1.0.0.tgz#602cd4b46e844ad4effc92a8011a3c46e0238dc2" integrity sha512-+q/t7Ekv1EDY2l6Gda6LLiX14rU9TV20Wa3ofeQmwPFZbOMo9DXrLbOjFaaclkXKWidIaopwAObQDqwWtGUjqg== -atob@^2.1.1: +atob@^2.1.1, atob@^2.1.2: version "2.1.2" resolved "https://registry.yarnpkg.com/atob/-/atob-2.1.2.tgz#6d9517eb9e030d2436666651e86bd9f6f13533c9" integrity sha512-Wm6ukoaOGJi/73p/cl2GvLjTI5JM1k/O14isD73YML8StrH/7/lRFgmg8nICZgD3bZZvjwCGxtMOD3wWNAu8cg== @@ -4494,6 +4556,14 @@ chalk@^1.1.1: strip-ansi "^3.0.0" supports-color "^2.0.0" +chalk@^3.0.0: + version "3.0.0" + resolved "https://registry.yarnpkg.com/chalk/-/chalk-3.0.0.tgz#3f73c2bf526591f574cc492c51e2456349f844e4" + integrity sha512-4D3B6Wf41KOYRFdszmDqMCGq5VV/uMAB273JILmO+3jAlh8X4qDtdtgCR3fxtbLEMzSx22QdhnDcJvu2u1fVwg== + dependencies: + ansi-styles "^4.1.0" + supports-color "^7.1.0" + chalk@^4.0.0: version "4.0.0" resolved "https://registry.yarnpkg.com/chalk/-/chalk-4.0.0.tgz#6e98081ed2d17faab615eb52ac66ec1fe6209e72" @@ -5236,6 +5306,20 @@ css-what@2.1, css-what@^2.1.2: resolved "https://registry.yarnpkg.com/css-what/-/css-what-2.1.3.tgz#a6d7604573365fe74686c3f311c56513d88285f2" integrity sha512-a+EPoD+uZiNfh+5fxw2nO9QwFa6nJe2Or35fGY6Ipw1R3R4AGz1d1TEZrCegvw2YTmZ0jXirGYlzxxpYSHwpEg== +css.escape@^1.5.1: + version "1.5.1" + resolved "https://registry.yarnpkg.com/css.escape/-/css.escape-1.5.1.tgz#42e27d4fa04ae32f931a4b4d4191fa9cddee97cb" + integrity sha1-QuJ9T6BK4y+TGktNQZH6nN3ul8s= + +css@^3.0.0: + version "3.0.0" + resolved "https://registry.yarnpkg.com/css/-/css-3.0.0.tgz#4447a4d58fdd03367c516ca9f64ae365cee4aa5d" + integrity sha512-DG9pFfwOrzc+hawpmqX/dHYHJG+Bsdb0klhyi1sDneOgGOXy9wQIC8hzyVp1e4NRYDBdxcylvywPkkXCHAzTyQ== + dependencies: + inherits "^2.0.4" + source-map "^0.6.1" + source-map-resolve "^0.6.0" + cssesc@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/cssesc/-/cssesc-2.0.0.tgz#3b13bd1bb1cb36e1bcb5a4dcd27f54c5dcb35703" @@ -5690,6 +5774,11 @@ doctrine@^3.0.0: dependencies: esutils "^2.0.2" +dom-accessibility-api@^0.5.4: + version "0.5.4" + resolved "https://registry.yarnpkg.com/dom-accessibility-api/-/dom-accessibility-api-0.5.4.tgz#b06d059cdd4a4ad9a79275f9d414a5c126241166" + integrity sha512-TvrjBckDy2c6v6RLxPv5QXOnU+SmF9nBII5621Ve5fu6Z/BDrENurBEvlC1f44lKEUVqOpK4w9E5Idc5/EgkLQ== + dom-align@^1.7.0: version "1.8.2" resolved "https://registry.yarnpkg.com/dom-align/-/dom-align-1.8.2.tgz#fdcd36bce25ba8d34fe3582efd57ac767df490bd" @@ -7923,7 +8012,7 @@ inflight@^1.0.4: once "^1.3.0" wrappy "1" -inherits@2: +inherits@2, inherits@^2.0.4: version "2.0.4" resolved "https://registry.yarnpkg.com/inherits/-/inherits-2.0.4.tgz#0fa2c64f932917c3433a0ded55363aae37416b7c" integrity sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ== @@ -9520,6 +9609,11 @@ lru-cache@~2.6.5: resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-2.6.5.tgz#e56d6354148ede8d7707b58d143220fd08df0fd5" integrity sha1-5W1jVBSO3o13B7WNFDIg/QjfD9U= +lz-string@^1.4.4: + version "1.4.4" + resolved "https://registry.yarnpkg.com/lz-string/-/lz-string-1.4.4.tgz#c0d8eaf36059f705796e1e344811cf4c498d3a26" + integrity sha1-wNjq82BZ9wV5bh40SBHPTEmNOiY= + make-dir@^2.1.0: version "2.1.0" resolved "https://registry.yarnpkg.com/make-dir/-/make-dir-2.1.0.tgz#5f0310e18b8be898cc07009295a30ae41e91e6f5" @@ -13346,6 +13440,14 @@ source-map-resolve@^0.5.0: source-map-url "^0.4.0" urix "^0.1.0" +source-map-resolve@^0.6.0: + version "0.6.0" + resolved "https://registry.yarnpkg.com/source-map-resolve/-/source-map-resolve-0.6.0.tgz#3d9df87e236b53f16d01e58150fc7711138e5ed2" + integrity sha512-KXBr9d/fO/bWo97NXsPIAW1bFSBOuCnjbNTBMO7N59hsv5i9yzRDfcYwwt0l04+VqnKC+EwzvJZIP/qkuMgR/w== + dependencies: + atob "^2.1.2" + decode-uri-component "^0.2.0" + source-map-support@^0.5.6: version "0.5.10" resolved "https://registry.yarnpkg.com/source-map-support/-/source-map-support-0.5.10.tgz#2214080bc9d51832511ee2bab96e3c2f9353120c" From 6b3810289c1737d34bd06485afce8e2f99dd40d6 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Mon, 30 Nov 2020 18:22:14 +0800 Subject: [PATCH 08/25] Rewrite GlobalSearchContainer tests with RTL Prepares GlobalSearchContainer tests for functionalization. The existing tests tested methods on the GlobalSearchContainer class, which were an implementation detail (those methods were meant to be called by the child `GlobalSearch` component). Those methods will be turned into `useCallback` callbacks in a functionalized component not accessible from outside the component, requiring a rewrite of the tests. --- .../layout/GlobalSearchContainer.test.tsx | 294 +++++++++++++++--- .../views/layout/GlobalSearchContainer.tsx | 3 +- 2 files changed, 255 insertions(+), 42 deletions(-) diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index 43cc7b5c90..033964aee4 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -1,4 +1,5 @@ -import { shallow } from 'enzyme'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import _ from 'lodash'; import { ModuleCondensed } from 'types/modules'; @@ -29,12 +30,12 @@ function make(props = {}) { ...props, }; - return shallow(); + return render(); } test('hides module when screen size is small', () => { - expect(make().isEmptyRender()).toBeFalsy(); - expect(make({ matchBreakpoint: false }).isEmptyRender()).toBeTruthy(); + expect(make().container).not.toBeEmptyDOMElement(); + expect(make({ matchBreakpoint: false }).container).toBeEmptyDOMElement(); }); test('fetches venue list', () => { @@ -44,59 +45,270 @@ test('fetches venue list', () => { }); test('shows no choices when search is too short', () => { - const instance = make().instance() as SearchContainerComponent; - expect(instance.getResults('1')).toBeNull(); -}); + make(); + + // Expect not to show choices when search string is too short + userEvent.type(screen.getByRole('textbox'), '1'); + expect(screen.queryAllByRole('option')).toHaveLength(0); -test('passes down search tokens', () => { - const instance = make().instance() as SearchContainerComponent; - expect(instance.getResults('ab')!.tokens).toEqual(['ab']); - expect(instance.getResults('a b')!.tokens).toEqual(['a', 'b']); - expect(instance.getResults('a, b')!.tokens).toEqual(['a', 'b']); - expect(instance.getResults(' a, b ')!.tokens).toEqual(['a', 'b']); + // Expect to show choices when search string is long enough + userEvent.type(screen.getByRole('textbox'), '1'); + expect(screen.queryAllByRole('option')).not.toHaveLength(0); }); test('shows at most 10 choices when there are many venues and modules', () => { - const instance = make().instance() as SearchContainerComponent; - const { modules, venues } = instance.getResults('1 ')!; - expect(modules).toHaveLength(6); - expect(venues).toHaveLength(4); + make(); + userEvent.type(screen.getByRole('textbox'), '1 '); + expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + Array [ + "ModulesView All ", + "AA1010 TestSem 1", + "AB1010 TestSem 1", + "AC1010 TestSem 1", + "AD1010 TestSem 1", + "AE1010 TestSem 1", + "AF1010 TestSem 1", + "VenuesView All ", + "AA-1", + "BB-1", + "CC-1", + "DD-1", + ] + `); }); -test('shows at most 10 choices when there are many venues', () => { - const instance = make({ - moduleList: MODULES.slice(0, 10), - venueList: VENUES.slice(0, 4), - }).instance() as SearchContainerComponent; - const { modules, venues } = instance.getResults('1 ')!; - expect(modules).toHaveLength(6); - expect(venues).toHaveLength(4); +test('prioritize showing venues when there are many venues even if there are modules', () => { + make({ + moduleList: MODULES.slice(0, 5), + }); + userEvent.type(screen.getByRole('textbox'), '1 '); + expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + Array [ + "VenuesView All ", + "AA-1", + "BB-1", + "CC-1", + "DD-1", + "EE-1", + "FF-1", + "GG-1", + "HH-1", + "II-1", + "JJ-1", + "KK-1", + "LL-1", + "MM-1", + "NN-1", + "OO-1", + "PP-1", + "QQ-1", + "RR-1", + "SS-1", + "TT-1", + "UU-1", + "VV-1", + "WW-1", + "XX-1", + "YY-1", + "ZZ-1", + ] + `); }); test('shows at most 10 choices when there are many modules', () => { - const instance = make({ venueList: VENUES.slice(0, 2) }).instance() as SearchContainerComponent; - const { modules, venues } = instance.getResults('1 ')!; - expect(modules).toHaveLength(8); - expect(venues).toHaveLength(2); + make({ + venueList: VENUES.slice(0, 2), + }); + userEvent.type(screen.getByRole('textbox'), '1 '); + expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + Array [ + "ModulesView All ", + "AA1010 TestSem 1", + "AB1010 TestSem 1", + "AC1010 TestSem 1", + "AD1010 TestSem 1", + "AE1010 TestSem 1", + "AF1010 TestSem 1", + "AG1010 TestSem 1", + "AH1010 TestSem 1", + "VenuesView All ", + "AA-1", + "BB-1", + ] + `); }); test('shows all results when there are few', () => { - const instance = make().instance() as SearchContainerComponent; - const { modules, venues } = instance.getResults('AA')!; - expect(modules).toHaveLength(1); - expect(venues).toHaveLength(1); + make(); + userEvent.type(screen.getByRole('textbox'), 'AA'); + expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + Array [ + "ModulesView All ", + "AA1010 TestSem 1", + "VenuesView All ", + "AA-1", + ] + `); }); test('show many results if the search only returns results of one type', () => { - const instance = make({ + make({ venueList: _.range(100).map((n) => `Venue ${n}`), - }).instance() as SearchContainerComponent; + }); + + userEvent.type(screen.getByRole('textbox'), '1010'); + expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + Array [ + "ModulesView All ", + "AA1010 TestSem 1", + "AB1010 TestSem 1", + "AC1010 TestSem 1", + "AD1010 TestSem 1", + "AE1010 TestSem 1", + "AF1010 TestSem 1", + "AG1010 TestSem 1", + "AH1010 TestSem 1", + "AI1010 TestSem 1", + "AJ1010 TestSem 1", + "AK1010 TestSem 1", + "AL1010 TestSem 1", + "AM1010 TestSem 1", + "AN1010 TestSem 1", + "AO1010 TestSem 1", + "AP1010 TestSem 1", + "AQ1010 TestSem 1", + "AR1010 TestSem 1", + "AS1010 TestSem 1", + "AT1010 TestSem 1", + "AU1010 TestSem 1", + "AV1010 TestSem 1", + "AW1010 TestSem 1", + "AX1010 TestSem 1", + "AY1010 TestSem 1", + "AZ1010 TestSem 1", + "BA1010 TestSem 1", + "BB1010 TestSem 1", + "BC1010 TestSem 1", + "BD1010 TestSem 1", + "BE1010 TestSem 1", + "BF1010 TestSem 1", + "BG1010 TestSem 1", + "BH1010 TestSem 1", + "BI1010 TestSem 1", + "BJ1010 TestSem 1", + "BK1010 TestSem 1", + "BL1010 TestSem 1", + "BM1010 TestSem 1", + "BN1010 TestSem 1", + "BO1010 TestSem 1", + "BP1010 TestSem 1", + "BQ1010 TestSem 1", + "BR1010 TestSem 1", + "BS1010 TestSem 1", + "BT1010 TestSem 1", + "BU1010 TestSem 1", + "BV1010 TestSem 1", + "BW1010 TestSem 1", + "BX1010 TestSem 1", + "BY1010 TestSem 1", + "BZ1010 TestSem 1", + "CA1010 TestSem 1", + "CB1010 TestSem 1", + "CC1010 TestSem 1", + "CD1010 TestSem 1", + "CE1010 TestSem 1", + "CF1010 TestSem 1", + "CG1010 TestSem 1", + "CH1010 TestSem 1", + "CI1010 TestSem 1", + "CJ1010 TestSem 1", + "CK1010 TestSem 1", + "CL1010 TestSem 1", + "CM1010 TestSem 1", + "CN1010 TestSem 1", + "CO1010 TestSem 1", + "CP1010 TestSem 1", + "CQ1010 TestSem 1", + "CR1010 TestSem 1", + ] + `); - let { modules, venues } = instance.getResults('1010')!; - expect(modules).toHaveLength(70); - expect(venues).toHaveLength(0); + userEvent.clear(screen.getByRole('textbox')); - ({ modules, venues } = instance.getResults('venue')!); - expect(modules).toHaveLength(0); - expect(venues).toHaveLength(70); + userEvent.type(screen.getByRole('textbox'), 'venue'); + expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + Array [ + "VenuesView All ", + "Venue 0", + "Venue 1", + "Venue 2", + "Venue 3", + "Venue 4", + "Venue 5", + "Venue 6", + "Venue 7", + "Venue 8", + "Venue 9", + "Venue 10", + "Venue 11", + "Venue 12", + "Venue 13", + "Venue 14", + "Venue 15", + "Venue 16", + "Venue 17", + "Venue 18", + "Venue 19", + "Venue 20", + "Venue 21", + "Venue 22", + "Venue 23", + "Venue 24", + "Venue 25", + "Venue 26", + "Venue 27", + "Venue 28", + "Venue 29", + "Venue 30", + "Venue 31", + "Venue 32", + "Venue 33", + "Venue 34", + "Venue 35", + "Venue 36", + "Venue 37", + "Venue 38", + "Venue 39", + "Venue 40", + "Venue 41", + "Venue 42", + "Venue 43", + "Venue 44", + "Venue 45", + "Venue 46", + "Venue 47", + "Venue 48", + "Venue 49", + "Venue 50", + "Venue 51", + "Venue 52", + "Venue 53", + "Venue 54", + "Venue 55", + "Venue 56", + "Venue 57", + "Venue 58", + "Venue 59", + "Venue 60", + "Venue 61", + "Venue 62", + "Venue 63", + "Venue 64", + "Venue 65", + "Venue 66", + "Venue 67", + "Venue 68", + "Venue 69", + ] + `); }); diff --git a/website/src/views/layout/GlobalSearchContainer.tsx b/website/src/views/layout/GlobalSearchContainer.tsx index 372155b001..661eaed04b 100644 --- a/website/src/views/layout/GlobalSearchContainer.tsx +++ b/website/src/views/layout/GlobalSearchContainer.tsx @@ -88,7 +88,8 @@ export class SearchContainerComponent extends Component { return { modules: modules.slice(0, 6), venues: venues.slice(0, 4), tokens }; } - // Some venues, show as many of them as possible as they are rare + // Either there are few modules, few venues, or both. + // If venues exist, show as many of them as possible as they are rare return { modules: modules.slice(0, RESULTS_LIMIT - venues.length), venues, tokens }; }; From 34c8bff4401b3192d54242e084ffc42cb92ed978 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 1 Dec 2020 15:37:31 +0800 Subject: [PATCH 09/25] Introduce __TEST__ global var --- website/jest.common.config.js | 3 ++- website/src/.eslintrc.js | 1 + website/src/bootstrapping/sentry.ts | 2 +- website/src/entry/main.tsx | 7 +++++-- website/src/types/global.d.ts | 4 +++- website/webpack/webpack.config.dev.js | 1 + website/webpack/webpack.config.prod.js | 1 + website/webpack/webpack.config.timetable-only.js | 3 ++- 8 files changed, 16 insertions(+), 6 deletions(-) diff --git a/website/jest.common.config.js b/website/jest.common.config.js index 1f4d8bd6d1..bf36984c5e 100644 --- a/website/jest.common.config.js +++ b/website/jest.common.config.js @@ -16,7 +16,8 @@ module.exports = { // Mimic the globals we set with Webpack's DefinePlugin globals: { // Default to development - __DEV__: process.env.NODE_ENV !== 'production', + __DEV__: process.env.NODE_ENV === 'development', + __TEST__: process.env.NODE_ENV === 'test', DATA_API_BASE_URL: '', VERSION_STR: '', DISPLAY_COMMIT_HASH: '', diff --git a/website/src/.eslintrc.js b/website/src/.eslintrc.js index 3586a6f4b1..cb17aa7737 100644 --- a/website/src/.eslintrc.js +++ b/website/src/.eslintrc.js @@ -105,6 +105,7 @@ module.exports = { // Mimic the globals we set with Webpack's DefinePlugin globals: { __DEV__: 'readonly', + __TEST__: 'readonly', DATA_API_BASE_URL: 'readonly', VERSION_STR: 'readonly', DISPLAY_COMMIT_HASH: 'readonly', diff --git a/website/src/bootstrapping/sentry.ts b/website/src/bootstrapping/sentry.ts index b3c2b065e3..36312f2128 100644 --- a/website/src/bootstrapping/sentry.ts +++ b/website/src/bootstrapping/sentry.ts @@ -4,7 +4,7 @@ import * as Sentry from '@sentry/browser'; import { isBrowserSupported } from './browser'; // Configure Raven - the client for Sentry, which we use to handle errors -const loadRaven = !__DEV__; +const loadRaven = !__DEV__ && !__TEST__; if (loadRaven) { Sentry.init({ dsn: 'https://4b4fe71954424fd39ac88a4f889ffe20@sentry.io/213986', diff --git a/website/src/entry/main.tsx b/website/src/entry/main.tsx index 8ccba1399c..baa1839471 100644 --- a/website/src/entry/main.tsx +++ b/website/src/entry/main.tsx @@ -28,13 +28,16 @@ ReactModal.setAppElement('#app'); ReactDOM.render(, document.getElementById('app')); if ( - (!__DEV__ && 'serviceWorker' in navigator && window.location.protocol === 'https:') || + (!__DEV__ && + !__TEST__ && + 'serviceWorker' in navigator && + window.location.protocol === 'https:') || // Allow us to force service worker to be enabled for debugging DEBUG_SERVICE_WORKER ) { registerServiceWorker(store); } -if (!__DEV__) { +if (!__DEV__ && !__TEST__) { initializeMamoto(); } diff --git a/website/src/types/global.d.ts b/website/src/types/global.d.ts index 6de4ca7287..76c1f88fa5 100644 --- a/website/src/types/global.d.ts +++ b/website/src/types/global.d.ts @@ -1,10 +1,12 @@ // Globals injected by Webpack DefinePlugin -// eslint-disable-next-line no-underscore-dangle +/* eslint-disable no-underscore-dangle */ declare const __DEV__: boolean; +declare const __TEST__: boolean; declare const DATA_API_BASE_URL: string | undefined; declare const VERSION_STR: string | undefined; declare const DISPLAY_COMMIT_HASH: string | undefined; declare const DEBUG_SERVICE_WORKER: boolean; +/* eslint-enable no-underscore-dangle */ /** * The declarations below let us use Webpack loaders to load non-JS files diff --git a/website/webpack/webpack.config.dev.js b/website/webpack/webpack.config.dev.js index c769bf3052..d2ad83f676 100644 --- a/website/webpack/webpack.config.dev.js +++ b/website/webpack/webpack.config.dev.js @@ -17,6 +17,7 @@ const developmentConfig = merge([ plugins: [ new webpack.DefinePlugin({ __DEV__: true, + __TEST__: false, DISPLAY_COMMIT_HASH: JSON.stringify(parts.appVersion().commitHash), VERSION_STR: JSON.stringify(parts.appVersion().versionStr), DEBUG_SERVICE_WORKER: !!process.env.DEBUG_SERVICE_WORKER, diff --git a/website/webpack/webpack.config.prod.js b/website/webpack/webpack.config.prod.js index c03744b068..8f20379f26 100644 --- a/website/webpack/webpack.config.prod.js +++ b/website/webpack/webpack.config.prod.js @@ -23,6 +23,7 @@ const productionConfig = ({ browserWarningPath }) => plugins: [ new webpack.DefinePlugin({ __DEV__: false, + __TEST__: false, DISPLAY_COMMIT_HASH: JSON.stringify(parts.appVersion().commitHash), VERSION_STR: JSON.stringify(parts.appVersion().versionStr), DEBUG_SERVICE_WORKER: !!process.env.DEBUG_SERVICE_WORKER, diff --git a/website/webpack/webpack.config.timetable-only.js b/website/webpack/webpack.config.timetable-only.js index 77802588ed..e50bd9906f 100644 --- a/website/webpack/webpack.config.timetable-only.js +++ b/website/webpack/webpack.config.timetable-only.js @@ -9,7 +9,7 @@ const commonConfig = require('./webpack.config.common'); const parts = require('./webpack.parts'); // eslint-disable-next-line no-underscore-dangle -const __DEV__ = process.env.NODE_ENV !== 'production'; +const __DEV__ = process.env.NODE_ENV === 'development'; const source = (file) => path.join('entry/export', file); @@ -18,6 +18,7 @@ const productionConfig = merge([ plugins: [ new webpack.DefinePlugin({ __DEV__, + __TEST__: process.env.NODE_ENV === 'test', DISPLAY_COMMIT_HASH: JSON.stringify(parts.appVersion().commitHash), VERSION_STR: JSON.stringify(parts.appVersion().versionStr), DEBUG_SERVICE_WORKER: !!process.env.DEBUG_SERVICE_WORKER, From 8846526b7c4be4d54c846fb153340cb322ee06e7 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 1 Dec 2020 15:39:12 +0800 Subject: [PATCH 10/25] Test GlobalSearchContainer wrapped in withRouter and connect HOCs Preparing to replace those HOCs with hooks. --- website/src/actions/venueBank.ts | 4 +- website/src/bootstrapping/configure-store.ts | 8 +- .../layout/GlobalSearchContainer.test.tsx | 157 +++++++++++------- .../views/layout/GlobalSearchContainer.tsx | 8 +- 4 files changed, 106 insertions(+), 71 deletions(-) diff --git a/website/src/actions/venueBank.ts b/website/src/actions/venueBank.ts index b8db3f4da6..8b5952d1fe 100644 --- a/website/src/actions/venueBank.ts +++ b/website/src/actions/venueBank.ts @@ -1,8 +1,8 @@ import { requestAction } from 'actions/requests'; import NUSModsApi from 'apis/nusmods'; import config from 'config'; -import { RequestActions } from 'middlewares/requests-middleware'; -import { VenueList } from 'types/venues'; +import type { RequestActions } from 'middlewares/requests-middleware'; +import type { VenueList } from 'types/venues'; export const FETCH_VENUE_LIST = 'FETCH_VENUE_LIST'; export function fetchVenueList() { diff --git a/website/src/bootstrapping/configure-store.ts b/website/src/bootstrapping/configure-store.ts index 05bc78f66f..d0f0f2149c 100644 --- a/website/src/bootstrapping/configure-store.ts +++ b/website/src/bootstrapping/configure-store.ts @@ -8,9 +8,9 @@ import requestsMiddleware from 'middlewares/requests-middleware'; import ravenMiddleware from 'middlewares/raven-middleware'; import getLocalStorage from 'storage/localStorage'; -import { GetState } from 'types/redux'; -import { State } from 'types/state'; -import { Actions } from 'types/actions'; +import type { GetState } from 'types/redux'; +import type { State } from 'types/state'; +import type { Actions } from 'types/actions'; // For redux-devtools-extensions - see // https://github.com/zalmoxisus/redux-devtools-extension @@ -36,7 +36,7 @@ export default function configureStore(defaultState?: State) { duration: true, diff: true, // Avoid diffing actions that insert a lot of stuff into the state to prevent console from lagging - diffPredicate: (getState: GetState, action: Actions) => + diffPredicate: (_getState: GetState, action: Actions) => !action.type.startsWith('FETCH_MODULE_LIST') && !action.type.startsWith('persist/'), }); middlewares.push(logger); diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index 033964aee4..c05a1be5e2 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -1,10 +1,19 @@ -import { render, screen } from '@testing-library/react'; +import { render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import _ from 'lodash'; +import { Provider } from 'react-redux'; +import { MemoryRouter } from 'react-router-dom'; +import produce from 'immer'; +import configureStore from 'bootstrapping/configure-store'; +import { GlobalSearchContainer } from 'views/layout/GlobalSearchContainer'; +import reducers from 'reducers'; +import { initAction } from 'test-utils/redux'; +import type { ModuleCondensed } from 'types/modules'; +import { fetchVenueList } from 'actions/venueBank'; -import { ModuleCondensed } from 'types/modules'; -import { SearchContainerComponent } from 'views/layout/GlobalSearchContainer'; -import createHistory from 'test-utils/createHistory'; +jest.mock('actions/venueBank'); + +const mockedFetchVenueList = fetchVenueList as jest.MockedFunction; const letters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'.split(''); @@ -20,46 +29,71 @@ const MODULES = _.flatMap(letters, (firstLetter): ModuleCondensed[] => // Produces 26 venues of the form AA-1, BB-1, CC-1, ... const VENUES = letters.map((letter) => `${letter}${letter}-1`); -function make(props = {}) { +const relevantStoreContents = { + moduleBank: { moduleList: MODULES }, + venueBank: { venueList: VENUES }, +}; + +function make(storeOverrides: Partial = {}, props = {}) { const allProps = { matchBreakpoint: true, - fetchVenueList: jest.fn(), - moduleList: MODULES, - venueList: VENUES, - ...createHistory(), ...props, }; - return render(); + const { store } = configureStore( + produce(reducers(undefined, initAction()), (draft) => { + draft.moduleBank.moduleList = (storeOverrides.moduleBank?.moduleList ?? + relevantStoreContents.moduleBank.moduleList) as typeof draft.moduleBank.moduleList; + draft.venueBank.venueList = (storeOverrides.venueBank?.venueList ?? + relevantStoreContents.venueBank.venueList) as typeof draft.venueBank.venueList; + }), + ); + + return render( + + + + + , + ); } +describe('GlobalSearchContainer', () => { + beforeEach(() => { + // Replace fetchVenueList with a noop action to stop it from firing API requests + mockedFetchVenueList.mockImplementation(() => initAction() as never); + }); -test('hides module when screen size is small', () => { - expect(make().container).not.toBeEmptyDOMElement(); - expect(make({ matchBreakpoint: false }).container).toBeEmptyDOMElement(); -}); + afterEach(() => { + mockedFetchVenueList.mockReset(); + }); -test('fetches venue list', () => { - const mock = jest.fn(); - make({ fetchVenueList: mock }); - expect(mock).toHaveBeenCalled(); -}); + test('hides module when screen size is small', () => { + expect(make().container).not.toBeEmptyDOMElement(); + expect(make({}, { matchBreakpoint: false }).container).toBeEmptyDOMElement(); + }); + + test('fetches venue list', () => { + expect(mockedFetchVenueList).not.toHaveBeenCalled(); + make(); + expect(mockedFetchVenueList).toHaveBeenCalled(); + }); -test('shows no choices when search is too short', () => { - make(); + test('shows no choices when search is too short', () => { + const { getByRole, queryAllByRole } = make(); - // Expect not to show choices when search string is too short - userEvent.type(screen.getByRole('textbox'), '1'); - expect(screen.queryAllByRole('option')).toHaveLength(0); + // Expect not to show choices when search string is too short + userEvent.type(getByRole('textbox'), '1'); + expect(queryAllByRole('option')).toHaveLength(0); - // Expect to show choices when search string is long enough - userEvent.type(screen.getByRole('textbox'), '1'); - expect(screen.queryAllByRole('option')).not.toHaveLength(0); -}); + // Expect to show choices when search string is long enough + userEvent.type(getByRole('textbox'), '1'); + expect(queryAllByRole('option')).not.toHaveLength(0); + }); -test('shows at most 10 choices when there are many venues and modules', () => { - make(); - userEvent.type(screen.getByRole('textbox'), '1 '); - expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + test('shows at most 10 choices when there are many venues and modules', () => { + const { getByRole, getAllByRole } = make(); + userEvent.type(getByRole('textbox'), '1 '); + expect(getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` Array [ "ModulesView All ", "AA1010 TestSem 1", @@ -75,14 +109,14 @@ test('shows at most 10 choices when there are many venues and modules', () => { "DD-1", ] `); -}); - -test('prioritize showing venues when there are many venues even if there are modules', () => { - make({ - moduleList: MODULES.slice(0, 5), }); - userEvent.type(screen.getByRole('textbox'), '1 '); - expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + + test('prioritize showing venues when there are many venues even if there are modules', () => { + const { getByRole, getAllByRole } = make({ + moduleBank: { moduleList: MODULES.slice(0, 5) }, + }); + userEvent.type(getByRole('textbox'), '1 '); + expect(getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` Array [ "VenuesView All ", "AA-1", @@ -113,14 +147,14 @@ test('prioritize showing venues when there are many venues even if there are mod "ZZ-1", ] `); -}); - -test('shows at most 10 choices when there are many modules', () => { - make({ - venueList: VENUES.slice(0, 2), }); - userEvent.type(screen.getByRole('textbox'), '1 '); - expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + + test('shows at most 10 choices when there are many modules', () => { + const { getByRole, getAllByRole } = make({ + venueBank: { venueList: VENUES.slice(0, 2) }, + }); + userEvent.type(getByRole('textbox'), '1 '); + expect(getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` Array [ "ModulesView All ", "AA1010 TestSem 1", @@ -136,12 +170,12 @@ test('shows at most 10 choices when there are many modules', () => { "BB-1", ] `); -}); + }); -test('shows all results when there are few', () => { - make(); - userEvent.type(screen.getByRole('textbox'), 'AA'); - expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + test('shows all results when there are few', () => { + const { getByRole, getAllByRole } = make(); + userEvent.type(getByRole('textbox'), 'AA'); + expect(getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` Array [ "ModulesView All ", "AA1010 TestSem 1", @@ -149,15 +183,15 @@ test('shows all results when there are few', () => { "AA-1", ] `); -}); - -test('show many results if the search only returns results of one type', () => { - make({ - venueList: _.range(100).map((n) => `Venue ${n}`), }); - userEvent.type(screen.getByRole('textbox'), '1010'); - expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + test('show many results if the search only returns results of one type', () => { + const { getByRole, getAllByRole } = make({ + venueBank: { venueList: _.range(100).map((n) => `Venue ${n}`) }, + }); + + userEvent.type(getByRole('textbox'), '1010'); + expect(getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` Array [ "ModulesView All ", "AA1010 TestSem 1", @@ -233,10 +267,10 @@ test('show many results if the search only returns results of one type', () => { ] `); - userEvent.clear(screen.getByRole('textbox')); + userEvent.clear(getByRole('textbox')); - userEvent.type(screen.getByRole('textbox'), 'venue'); - expect(screen.getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` + userEvent.type(getByRole('textbox'), 'venue'); + expect(getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` Array [ "VenuesView All ", "Venue 0", @@ -311,4 +345,5 @@ test('show many results if the search only returns results of one type', () => { "Venue 69", ] `); + }); }); diff --git a/website/src/views/layout/GlobalSearchContainer.tsx b/website/src/views/layout/GlobalSearchContainer.tsx index 661eaed04b..5d72237473 100644 --- a/website/src/views/layout/GlobalSearchContainer.tsx +++ b/website/src/views/layout/GlobalSearchContainer.tsx @@ -28,7 +28,7 @@ const RESULTS_LIMIT = 10; const LONG_LIST_LIMIT = 70; const MIN_INPUT_LENGTH = 2; -export class SearchContainerComponent extends Component { +class GlobalSearchContainerComponent extends Component { componentDidMount() { this.props.fetchVenueList(); } @@ -107,8 +107,8 @@ export class SearchContainerComponent extends Component { } } -const routedSearchContainer = withRouter(SearchContainerComponent); -const connectedSearchContainer = connect( +const routedSearchContainer = withRouter(GlobalSearchContainerComponent); +export const GlobalSearchContainer = connect( (state: State) => ({ moduleList: state.moduleBank.moduleList, venueList: state.venueBank.venueList, @@ -116,4 +116,4 @@ const connectedSearchContainer = connect( { fetchVenueList }, )(routedSearchContainer); -export default makeResponsive(connectedSearchContainer, breakpointUp('md')); +export default makeResponsive(GlobalSearchContainer, breakpointUp('md')); From 87ec3acf769e851c1ecd782cca905ee4d614dbd8 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 1 Dec 2020 15:59:47 +0800 Subject: [PATCH 11/25] Functionalize GlobalSearchContainer --- .../layout/GlobalSearchContainer.test.tsx | 2 +- .../views/layout/GlobalSearchContainer.tsx | 195 +++++++++--------- 2 files changed, 94 insertions(+), 103 deletions(-) diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index c05a1be5e2..fab2c8076d 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -57,7 +57,7 @@ function make(storeOverrides: Partial = {}, props , ); } -describe('GlobalSearchContainer', () => { +describe(GlobalSearchContainer, () => { beforeEach(() => { // Replace fetchVenueList with a noop action to stop it from firing API requests mockedFetchVenueList.mockImplementation(() => initAction() as never); diff --git a/website/src/views/layout/GlobalSearchContainer.tsx b/website/src/views/layout/GlobalSearchContainer.tsx index 5d72237473..df72badaeb 100644 --- a/website/src/views/layout/GlobalSearchContainer.tsx +++ b/website/src/views/layout/GlobalSearchContainer.tsx @@ -1,11 +1,11 @@ -import { ModuleCondensed } from 'types/modules'; -import { Venue, VenueList } from 'types/venues'; -import { ModuleList } from 'types/reducers'; +import type { ModuleCondensed } from 'types/modules'; +import type { Venue } from 'types/venues'; +import type { State } from 'types/state'; import { ResultType, SearchResult, VENUE_RESULT } from 'types/views'; -import { Component } from 'react'; -import { connect } from 'react-redux'; -import { RouteComponentProps, withRouter } from 'react-router-dom'; +import { FC, useCallback, useEffect } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { useHistory } from 'react-router-dom'; import GlobalSearch from 'views/layout/GlobalSearch'; import { modulePage, venuePage } from 'views/routes/paths'; @@ -14,106 +14,97 @@ import { createSearchPredicate, regexify, sortModules, tokenize } from 'utils/mo import { breakpointUp } from 'utils/css'; import { takeUntil } from 'utils/array'; import makeResponsive from 'views/hocs/makeResponsive'; -import { State } from 'types/state'; - -type Props = RouteComponentProps & { - moduleList: ModuleList; - venueList: VenueList; - matchBreakpoint: boolean; - - fetchVenueList: () => void; -}; const RESULTS_LIMIT = 10; const LONG_LIST_LIMIT = 70; const MIN_INPUT_LENGTH = 2; -class GlobalSearchContainerComponent extends Component { - componentDidMount() { - this.props.fetchVenueList(); - } - - shouldComponentUpdate(nextProps: Props) { - return ( - this.props.matchBreakpoint !== nextProps.matchBreakpoint || - this.props.moduleList.length !== nextProps.moduleList.length || - this.props.venueList.length !== nextProps.venueList.length - ); - } - - onSelectModule = (module: ModuleCondensed) => { - this.props.history.push(modulePage(module.moduleCode, module.title)); - }; - - onSelectVenue = (venue: Venue) => { - this.props.history.push(venuePage(venue)); - }; - - onSearch = (type: ResultType, query: string) => { - // TODO: Move this into a proper function - const path = type === VENUE_RESULT ? '/venues' : '/modules'; - this.props.history.push(`${path}?q=${encodeURIComponent(query)}`); - }; - - getResults = (inputValue: string | null): SearchResult | null => { - if (!inputValue || inputValue.length < MIN_INPUT_LENGTH) { - return null; - } - - const { moduleList, venueList } = this.props; - const tokens = tokenize(inputValue); - - // Filter venues - const regex = regexify(inputValue); - const venues = takeUntil(venueList, LONG_LIST_LIMIT, (venue) => regex.test(venue)); - - // Filter modules - const predicate = createSearchPredicate(inputValue); - const filteredModules = takeUntil(moduleList, LONG_LIST_LIMIT, (module) => - predicate({ ...module }), - ); - const modules = sortModules(inputValue, filteredModules.slice()); - - // There's only one type of result - use the long list format - if (!modules.length || !venues.length) { - return { - modules: modules.slice(0, LONG_LIST_LIMIT), - venues: venues.slice(0, LONG_LIST_LIMIT), - tokens, - }; - } - - // Plenty of modules and venues, show 6 modules, 4 venues - if (modules.length >= 6 && venues.length >= 4) { - return { modules: modules.slice(0, 6), venues: venues.slice(0, 4), tokens }; - } - - // Either there are few modules, few venues, or both. - // If venues exist, show as many of them as possible as they are rare - return { modules: modules.slice(0, RESULTS_LIMIT - venues.length), venues, tokens }; - }; - - render() { - const { matchBreakpoint } = this.props; - if (!matchBreakpoint) return null; - return ( - - ); - } -} - -const routedSearchContainer = withRouter(GlobalSearchContainerComponent); -export const GlobalSearchContainer = connect( - (state: State) => ({ - moduleList: state.moduleBank.moduleList, - venueList: state.venueBank.venueList, - }), - { fetchVenueList }, -)(routedSearchContainer); +type Props = { + matchBreakpoint: boolean; +}; + +export const GlobalSearchContainer: FC = ({ matchBreakpoint }) => { + const dispatch = useDispatch(); + useEffect(() => { + dispatch(fetchVenueList()); + }, [dispatch]); + + const history = useHistory(); + const onSelectModule = useCallback( + (module: ModuleCondensed) => { + history.push(modulePage(module.moduleCode, module.title)); + }, + [history], + ); + + const onSelectVenue = useCallback( + (venue: Venue) => { + history.push(venuePage(venue)); + }, + [history], + ); + + const onSearch = useCallback( + (type: ResultType, query: string) => { + // TODO: Move this into a proper function + const path = type === VENUE_RESULT ? '/venues' : '/modules'; + history.push(`${path}?q=${encodeURIComponent(query)}`); + }, + [history], + ); + + const moduleList = useSelector(({ moduleBank }: State) => moduleBank.moduleList); + const venueList = useSelector(({ venueBank }: State) => venueBank.venueList); + + const getResults = useCallback( + (inputValue: string | null): SearchResult | null => { + if (!inputValue || inputValue.length < MIN_INPUT_LENGTH) { + return null; + } + + const tokens = tokenize(inputValue); + + // Filter venues + const regex = regexify(inputValue); + const venues = takeUntil(venueList, LONG_LIST_LIMIT, (venue) => regex.test(venue)); + + // Filter modules + const predicate = createSearchPredicate(inputValue); + const filteredModules = takeUntil(moduleList, LONG_LIST_LIMIT, (module) => + predicate({ ...module }), + ); + const modules = sortModules(inputValue, filteredModules.slice()); + + // There's only one type of result - use the long list format + if (!modules.length || !venues.length) { + return { + modules: modules.slice(0, LONG_LIST_LIMIT), + venues: venues.slice(0, LONG_LIST_LIMIT), + tokens, + }; + } + + // Plenty of modules and venues, show 6 modules, 4 venues + if (modules.length >= 6 && venues.length >= 4) { + return { modules: modules.slice(0, 6), venues: venues.slice(0, 4), tokens }; + } + + // Either there are few modules, few venues, or both. + // If venues exist, show as many of them as possible as they are rare + return { modules: modules.slice(0, RESULTS_LIMIT - venues.length), venues, tokens }; + }, + [moduleList, venueList], + ); + + if (!matchBreakpoint) return null; + return ( + + ); +}; export default makeResponsive(GlobalSearchContainer, breakpointUp('md')); From 08d7e1f744488628e7157196d0bff1b41637cabe Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 1 Dec 2020 17:03:36 +0800 Subject: [PATCH 12/25] Define new useMediaQuery hook to replace makeResponsive HOC --- website/package.json | 10 ++-- website/src/utils/css.ts | 6 +-- website/src/views/hocs/makeResponsive.tsx | 63 ++++++----------------- website/src/views/hooks/useMediaQuery.ts | 34 ++++++++++++ website/yarn.lock | 24 ++++++--- 5 files changed, 75 insertions(+), 62 deletions(-) create mode 100644 website/src/views/hooks/useMediaQuery.ts diff --git a/website/package.json b/website/package.json index a985869274..939793effa 100644 --- a/website/package.json +++ b/website/package.json @@ -37,9 +37,9 @@ "@packtracker/webpack-plugin": "2.3.0", "@pmmmwh/react-refresh-webpack-plugin": "0.4.3", "@svgr/webpack": "5.5.0", - "@testing-library/jest-dom": "^5.11.6", - "@testing-library/react": "^11.2.2", - "@testing-library/user-event": "^12.2.2", + "@testing-library/jest-dom": "5.11.6", + "@testing-library/react": "11.2.2", + "@testing-library/user-event": "12.3.0", "@types/classnames": "2.2.11", "@types/enzyme": "3.10.8", "@types/jest": "26.0.15", @@ -60,6 +60,7 @@ "@types/react-router-dom": "5.1.6", "@types/react-scrollspy": "3.3.3", "@types/redux-mock-store": "1.0.2", + "@types/use-subscription": "1.0.0", "@types/webpack-env": "1.16.0", "@typescript-eslint/eslint-plugin": "4.8.2", "@typescript-eslint/parser": "4.8.2", @@ -168,7 +169,8 @@ "redux-persist": "6.0.0", "redux-thunk": "2.3.0", "reselect": "4.0.0", - "searchkit": "2.4.1-alpha.5" + "searchkit": "2.4.1-alpha.5", + "use-subscription": "1.5.1" }, "browserslist": [ "extends browserslist-config-nusmods" diff --git a/website/src/utils/css.ts b/website/src/utils/css.ts index 0b430e8ca7..8abb58a956 100644 --- a/website/src/utils/css.ts +++ b/website/src/utils/css.ts @@ -1,5 +1,5 @@ // Define media breakpoints -import json2mq, { QueryObject } from 'json2mq'; +import type { QueryObject } from 'json2mq'; import { entries } from 'lodash'; export type Breakpoint = 'xs' | 'sm' | 'md' | 'lg' | 'xl'; @@ -34,10 +34,6 @@ export function touchScreenOnly(): QueryObject { return { pointer: 'coarse' }; } -export function queryMatch(query: QueryObject) { - return window.matchMedia(json2mq(query)); -} - export function supportsCSSVariables() { // Safari does not support supports('--var', 'red') return CSS.supports && CSS.supports('(--var: red)'); diff --git a/website/src/views/hocs/makeResponsive.tsx b/website/src/views/hocs/makeResponsive.tsx index 12c436eeff..b127bd567a 100644 --- a/website/src/views/hocs/makeResponsive.tsx +++ b/website/src/views/hocs/makeResponsive.tsx @@ -1,54 +1,23 @@ -import * as React from 'react'; -import json2mq, { QueryObject } from 'json2mq'; +import type { ComponentType } from 'react'; +import type { QueryObject } from 'json2mq'; -import { wrapComponentName } from 'utils/react'; +import useMediaQuery from 'views/hooks/useMediaQuery'; -export interface WithBreakpoint { +export type WithBreakpoint = { matchBreakpoint: boolean; -} +}; -function makeResponsive( - WrappedComponent: React.ComponentType, +/** + * @deprecated Use `useMediaQuery` instead. + */ +export default function makeResponsive( + WrappedComponent: ComponentType, mediaQuery: string | QueryObject | QueryObject[], -): React.ComponentType> { - const media = typeof mediaQuery === 'string' ? mediaQuery : json2mq(mediaQuery); - - return class extends React.Component, WithBreakpoint> { - mql: MediaQueryList | null = null; - - static displayName = wrapComponentName(WrappedComponent, makeResponsive.name); - - state = { - matchBreakpoint: false, - }; - - componentDidMount() { - const mql = window.matchMedia(media); - mql.addListener(this.updateMediaQuery); - this.updateMediaQuery(mql); - this.mql = mql; - } - - componentWillUnmount() { - if (this.mql) { - this.mql.removeListener(this.updateMediaQuery); - } - } - - updateMediaQuery = (e: MediaQueryListEvent | MediaQueryList) => { - if (e.matches !== this.state.matchBreakpoint) { - this.setState({ matchBreakpoint: e.matches }); - } - }; - - render() { - return ( - // TODO: remove as Props hack as defined in: - // https://github.com/Microsoft/TypeScript/issues/28938#issuecomment-450636046 - - ); - } +): ComponentType> { + return (props) => { + const matchBreakpoint = useMediaQuery(mediaQuery); + // TODO: remove as Props hack as defined in: + // https://github.com/Microsoft/TypeScript/issues/28938#issuecomment-450636046 + return ; }; } - -export default makeResponsive; diff --git a/website/src/views/hooks/useMediaQuery.ts b/website/src/views/hooks/useMediaQuery.ts new file mode 100644 index 0000000000..fff951a5d3 --- /dev/null +++ b/website/src/views/hooks/useMediaQuery.ts @@ -0,0 +1,34 @@ +import { useMemo } from 'react'; +import { Subscription, useSubscription } from 'use-subscription'; +import json2mq, { QueryObject } from 'json2mq'; + +/** + * To be used together with utilities in css.ts. + * @returns Whether `mediaQuery` is/are matched. + */ +export default function useMediaQuery(mediaQuery: string | QueryObject | QueryObject[]) { + const media = useMemo(() => { + return typeof mediaQuery === 'string' ? mediaQuery : json2mq(mediaQuery); + }, [mediaQuery]); + + const subscription = useMemo>( + () => ({ + getCurrentValue: () => window.matchMedia(media).matches, + subscribe(callback) { + const mediaQueryList = window.matchMedia(media); + mediaQueryList.addEventListener('change', callback); + return () => { + mediaQueryList.removeEventListener('change', callback); + }; + }, + }), + [media], + ); + + // TODO: Replace with useMutableSource after we adopt React concurrent mode. See: + // Docs: https://github.com/bvaughn/rfcs/blob/useMutableSource/text/0000-use-mutable-source.md + // Example: https://github.com/facebook/react/blob/36df483/packages/react-devtools-scheduling-profiler/src/hooks.js + const matchedBreakpoint = useSubscription(subscription); + + return matchedBreakpoint; +} diff --git a/website/yarn.lock b/website/yarn.lock index bd1cb6faab..1eff69526e 100644 --- a/website/yarn.lock +++ b/website/yarn.lock @@ -2534,7 +2534,7 @@ lz-string "^1.4.4" pretty-format "^26.6.2" -"@testing-library/jest-dom@^5.11.6": +"@testing-library/jest-dom@5.11.6": version "5.11.6" resolved "https://registry.yarnpkg.com/@testing-library/jest-dom/-/jest-dom-5.11.6.tgz#782940e82e5cd17bc0a36f15156ba16f3570ac81" integrity sha512-cVZyUNRWwUKI0++yepYpYX7uhrP398I+tGz4zOlLVlUYnZS+Svuxv4fwLeCIy7TnBYKXUaOlQr3vopxL8ZfEnA== @@ -2548,7 +2548,7 @@ lodash "^4.17.15" redent "^3.0.0" -"@testing-library/react@^11.2.2": +"@testing-library/react@11.2.2": version "11.2.2" resolved "https://registry.yarnpkg.com/@testing-library/react/-/react-11.2.2.tgz#099c6c195140ff069211143cb31c0f8337bdb7b7" integrity sha512-jaxm0hwUjv+hzC+UFEywic7buDC9JQ1q3cDsrWVSDAPmLotfA6E6kUHlYm/zOeGCac6g48DR36tFHxl7Zb+N5A== @@ -2556,10 +2556,10 @@ "@babel/runtime" "^7.12.5" "@testing-library/dom" "^7.28.1" -"@testing-library/user-event@^12.2.2": - version "12.2.2" - resolved "https://registry.yarnpkg.com/@testing-library/user-event/-/user-event-12.2.2.tgz#22d0047da745289335240f523dfe74c889ec96cb" - integrity sha512-mTYL9LrwiSeyorStUOMuRGQDn1ca40tIhuv//o/K3lY8wBEp+9Im90MFVx5i3u7zCPmavn3uWZs/10chsbI8Tg== +"@testing-library/user-event@12.3.0": + version "12.3.0" + resolved "https://registry.yarnpkg.com/@testing-library/user-event/-/user-event-12.3.0.tgz#bd4edf544b70721e1996f3be36830e942c777b2b" + integrity sha512-A4TZofjkOH42ydTtHZcGNhwYjonkVIGBi4pmNweUgjDEGmWHuZf4k7hLd6QTL+rkSOgrd3TOCFDNyK9KO0reeA== dependencies: "@babel/runtime" "^7.10.2" @@ -3002,6 +3002,11 @@ resolved "https://registry.yarnpkg.com/@types/unist/-/unist-2.0.3.tgz#9c088679876f374eb5983f150d4787aa6fb32d7e" integrity sha512-FvUupuM3rlRsRtCN+fDudtmytGO6iHJuuRKS1Ss0pG5z8oX0diNEw94UEL7hgDbpN94rgaK5R7sWm6RrSkZuAQ== +"@types/use-subscription@1.0.0": + version "1.0.0" + resolved "https://registry.yarnpkg.com/@types/use-subscription/-/use-subscription-1.0.0.tgz#d146f8d834f70f50d48bd8246a481d096f11db19" + integrity sha512-0WWZ5GUDKMXUY/1zy4Ur5/zsC0s/B+JjXfHdkvx6JgDNZzZV5eW+KKhDqsTGyqX56uh99gwGwbsKbVwkcVIKQA== + "@types/webpack-env@1.16.0": version "1.16.0" resolved "https://registry.yarnpkg.com/@types/webpack-env/-/webpack-env-1.16.0.tgz#8c0a9435dfa7b3b1be76562f3070efb3f92637b4" @@ -14737,6 +14742,13 @@ use-memo-one@^1.1.1: resolved "https://registry.yarnpkg.com/use-memo-one/-/use-memo-one-1.1.1.tgz#39e6f08fe27e422a7d7b234b5f9056af313bd22c" integrity sha512-oFfsyun+bP7RX8X2AskHNTxu+R3QdE/RC5IefMbqptmACAA/gfol1KDD5KRzPsGMa62sWxGZw+Ui43u6x4ddoQ== +use-subscription@1.5.1: + version "1.5.1" + resolved "https://registry.yarnpkg.com/use-subscription/-/use-subscription-1.5.1.tgz#73501107f02fad84c6dd57965beb0b75c68c42d1" + integrity sha512-Xv2a1P/yReAjAbhylMfFplFKj9GssgTwN7RlcTxBujFQcloStWNDQdc4g4NRWH9xS4i/FDk04vQBptAXoF3VcA== + dependencies: + object-assign "^4.1.1" + use@^3.1.0: version "3.1.1" resolved "https://registry.yarnpkg.com/use/-/use-3.1.1.tgz#d50c8cac79a19fbc20f2911f56eb973f4e10070f" From 2008c60b5005fde270e586861a5f9d64ed12faf3 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 1 Dec 2020 17:52:25 +0800 Subject: [PATCH 13/25] Make mockDom mock more robust; mock matchMedia in GlobalSearchContainer tests --- website/src/test-utils/mockDom.ts | 38 +++++++++++++++++-- .../src/views/components/ScrollToTop.test.tsx | 6 ++- .../layout/GlobalSearchContainer.test.tsx | 34 +++++++++++------ 3 files changed, 62 insertions(+), 16 deletions(-) diff --git a/website/src/test-utils/mockDom.ts b/website/src/test-utils/mockDom.ts index 2cb1e8a218..8e773852d4 100644 --- a/website/src/test-utils/mockDom.ts +++ b/website/src/test-utils/mockDom.ts @@ -1,13 +1,19 @@ -export default function mockDom() { +const nativeScrollTo = window.scrollTo; +const nativePerformance = window.performance; +const nativeMatchMedia = window.matchMedia; +const nativeScrollIntoView = Element.prototype.scrollIntoView; + +export function mockDom() { // Mock some of the DOM environment functions that are missing from JSDom window.scrollTo = jest.fn(); if (!window.performance) { - (window as any).performance = { now: jest.fn() }; + // @ts-expect-error We insist + window.performance = { now: jest.fn() }; } if (!window.matchMedia) { - (window as any).matchMedia = jest.fn(() => ({ matches: jest.fn(), addListener: jest.fn() })); + mockWindowMatchMedia(); } // JSDom does not stub scrollIntoView - https://github.com/jsdom/jsdom/issues/1695 @@ -15,3 +21,29 @@ export default function mockDom() { Element.prototype.scrollIntoView = jest.fn(); } } + +export function mockDomReset() { + window.scrollTo = nativeScrollTo; + + // @ts-expect-error We insist + window.performance = nativePerformance; + + window.matchMedia = nativeMatchMedia; + + Element.prototype.scrollIntoView = nativeScrollIntoView; +} + +export function mockWindowMatchMedia(overrides: Partial = {}) { + // Source: https://jestjs.io/docs/en/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom + window.matchMedia = jest.fn((query) => ({ + matches: true, + media: query, + onchange: null, + addListener: jest.fn(), // deprecated + removeListener: jest.fn(), // deprecated + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), + ...overrides, + })); +} diff --git a/website/src/views/components/ScrollToTop.test.tsx b/website/src/views/components/ScrollToTop.test.tsx index c4c8367fb8..b33a2b0a37 100644 --- a/website/src/views/components/ScrollToTop.test.tsx +++ b/website/src/views/components/ScrollToTop.test.tsx @@ -2,7 +2,7 @@ import { act } from 'react-dom/test-utils'; import { Router } from 'react-router-dom'; import { mount } from 'enzyme'; import createHistory from 'test-utils/createHistory'; -import mockDom from 'test-utils/mockDom'; +import { mockDom, mockDomReset } from 'test-utils/mockDom'; import ScrollToTop from './ScrollToTop'; @@ -16,6 +16,10 @@ describe('ScrollToTopComponent', () => { mockDom(); }); + afterEach(() => { + mockDomReset(); + }); + function make(props: Props = {}) { const { history } = createHistory(); act(() => { diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index fab2c8076d..6258bf1fe3 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -1,15 +1,16 @@ -import { render } from '@testing-library/react'; +import { act, render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import _ from 'lodash'; import { Provider } from 'react-redux'; import { MemoryRouter } from 'react-router-dom'; import produce from 'immer'; import configureStore from 'bootstrapping/configure-store'; -import { GlobalSearchContainer } from 'views/layout/GlobalSearchContainer'; import reducers from 'reducers'; -import { initAction } from 'test-utils/redux'; import type { ModuleCondensed } from 'types/modules'; import { fetchVenueList } from 'actions/venueBank'; +import GlobalSearchContainer from 'views/layout/GlobalSearchContainer'; +import { initAction } from 'test-utils/redux'; +import { mockDom, mockDomReset, mockWindowMatchMedia } from 'test-utils/mockDom'; jest.mock('actions/venueBank'); @@ -34,12 +35,7 @@ const relevantStoreContents = { venueBank: { venueList: VENUES }, }; -function make(storeOverrides: Partial = {}, props = {}) { - const allProps = { - matchBreakpoint: true, - ...props, - }; - +function make(storeOverrides: Partial = {}) { const { store } = configureStore( produce(reducers(undefined, initAction()), (draft) => { draft.moduleBank.moduleList = (storeOverrides.moduleBank?.moduleList ?? @@ -52,24 +48,38 @@ function make(storeOverrides: Partial = {}, props return render( - + , ); } describe(GlobalSearchContainer, () => { beforeEach(() => { + mockDom(); + // Replace fetchVenueList with a noop action to stop it from firing API requests mockedFetchVenueList.mockImplementation(() => initAction() as never); }); afterEach(() => { mockedFetchVenueList.mockReset(); + mockDomReset(); }); test('hides module when screen size is small', () => { - expect(make().container).not.toBeEmptyDOMElement(); - expect(make({}, { matchBreakpoint: false }).container).toBeEmptyDOMElement(); + let onMediaChangeCallback: () => void; + const addEventListener = (_type: string, listener: (...args: unknown[]) => void) => { + onMediaChangeCallback = listener; + }; + + mockWindowMatchMedia({ matches: true, addEventListener }); + const { container } = make(); + expect(container).not.toBeEmptyDOMElement(); + + mockWindowMatchMedia({ matches: false, addEventListener }); + // Trigger render when matches changes + act(() => onMediaChangeCallback()); + expect(container).toBeEmptyDOMElement(); }); test('fetches venue list', () => { From aea48bb757c660aa42c4d892de64c9256f8528f4 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 1 Dec 2020 17:54:52 +0800 Subject: [PATCH 14/25] Replace makeResponsive -> useMediaQuery in GlobalSearchContainer --- .../src/views/layout/GlobalSearchContainer.tsx | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/website/src/views/layout/GlobalSearchContainer.tsx b/website/src/views/layout/GlobalSearchContainer.tsx index df72badaeb..b05fb01133 100644 --- a/website/src/views/layout/GlobalSearchContainer.tsx +++ b/website/src/views/layout/GlobalSearchContainer.tsx @@ -1,29 +1,26 @@ import type { ModuleCondensed } from 'types/modules'; import type { Venue } from 'types/venues'; import type { State } from 'types/state'; -import { ResultType, SearchResult, VENUE_RESULT } from 'types/views'; import { FC, useCallback, useEffect } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { useHistory } from 'react-router-dom'; + +import useMediaQuery from 'views/hooks/useMediaQuery'; import GlobalSearch from 'views/layout/GlobalSearch'; import { modulePage, venuePage } from 'views/routes/paths'; +import { ResultType, SearchResult, VENUE_RESULT } from 'types/views'; import { fetchVenueList } from 'actions/venueBank'; import { createSearchPredicate, regexify, sortModules, tokenize } from 'utils/moduleSearch'; import { breakpointUp } from 'utils/css'; import { takeUntil } from 'utils/array'; -import makeResponsive from 'views/hocs/makeResponsive'; const RESULTS_LIMIT = 10; const LONG_LIST_LIMIT = 70; const MIN_INPUT_LENGTH = 2; -type Props = { - matchBreakpoint: boolean; -}; - -export const GlobalSearchContainer: FC = ({ matchBreakpoint }) => { +const GlobalSearchContainer: FC = () => { const dispatch = useDispatch(); useEffect(() => { dispatch(fetchVenueList()); @@ -96,7 +93,9 @@ export const GlobalSearchContainer: FC = ({ matchBreakpoint }) => { [moduleList, venueList], ); - if (!matchBreakpoint) return null; + const matchedBreakpoint = useMediaQuery(breakpointUp('md')); + + if (!matchedBreakpoint) return null; return ( = ({ matchBreakpoint }) => { ); }; -export default makeResponsive(GlobalSearchContainer, breakpointUp('md')); +export default GlobalSearchContainer; From 19e9e08e66748bab8cdbd39ef927351ca6e14c45 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Tue, 1 Dec 2020 17:58:00 +0800 Subject: [PATCH 15/25] Fix LessonTimetable type error --- website/src/views/components/module-info/LessonTimetable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/src/views/components/module-info/LessonTimetable.tsx b/website/src/views/components/module-info/LessonTimetable.tsx index bac2eb7b4e..4e72996587 100644 --- a/website/src/views/components/module-info/LessonTimetable.tsx +++ b/website/src/views/components/module-info/LessonTimetable.tsx @@ -36,7 +36,7 @@ const SemesterLessonTimetable: FC<{ semesterData?: SemesterData }> = ({ semester ); }; -const LessonTimetable: FC<{ allSemesterData: SemesterData[] }> = ({ allSemesterData }) => { +const LessonTimetable: FC<{ allSemesterData: readonly SemesterData[] }> = ({ allSemesterData }) => { const [selectedSem, setSelectedSem] = useState(() => getFirstAvailableSemester(allSemesterData)); return ( <> From b31bc83529e0a2cea8bcd9b87b460f16cbbc23f9 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 12:35:51 +0800 Subject: [PATCH 16/25] Add @testing-library/* to Renovate test group --- renovate.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/renovate.json b/renovate.json index 05ba916c66..b88e5d9725 100644 --- a/renovate.json +++ b/renovate.json @@ -42,7 +42,7 @@ }, { "extends": "monorepo:jest", - "packagePatterns": ["(jest|enzyme|mock)"], + "packagePatterns": ["(jest|enzyme|mock)", "^@testing-library/*"], "packageNames": ["identity-obj-proxy"], "groupName": "test" }, From 10e4f492530162cb84c3dcaec9a9574993668672 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 12:56:45 +0800 Subject: [PATCH 17/25] Fix nits in css.ts --- website/src/utils/css.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/website/src/utils/css.ts b/website/src/utils/css.ts index 8abb58a956..3a690eefc3 100644 --- a/website/src/utils/css.ts +++ b/website/src/utils/css.ts @@ -1,19 +1,18 @@ -// Define media breakpoints import type { QueryObject } from 'json2mq'; -import { entries } from 'lodash'; -export type Breakpoint = 'xs' | 'sm' | 'md' | 'lg' | 'xl'; - -const breakpoints: { [breakpoint: string]: number } = { +// NOTE: Keep in sync with Bootstrap's breakpoints. +// Breakpoints at time of writing: https://getbootstrap.com/docs/4.5/layout/overview/ +const breakpoints = Object.freeze({ xs: 0, sm: 576, md: 768, lg: 992, xl: 1200, -}; +}); +type Breakpoint = keyof typeof breakpoints; -function nextBreakpoint(size: Breakpoint): number | null | undefined { - const breakpointEntries = entries(breakpoints); +function nextBreakpoint(size: Breakpoint): number | null { + const breakpointEntries = Object.entries(breakpoints); const nextBreakpointIndex = breakpointEntries.findIndex(([breakpoint]) => breakpoint === size) + 1; if (nextBreakpointIndex >= breakpointEntries.length) return null; @@ -22,7 +21,7 @@ function nextBreakpoint(size: Breakpoint): number | null | undefined { export function breakpointDown(size: Breakpoint): QueryObject { const nextSize = nextBreakpoint(size); - if (nextSize == null) return { all: true }; + if (nextSize === null) return { all: true }; return { maxWidth: nextSize - 1 }; } From d925401880cda3fcf6c9f17ba646545295afd343 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 13:03:15 +0800 Subject: [PATCH 18/25] Migrate ScrollToTop from enzyme -> RTL --- .../src/views/components/ScrollToTop.test.tsx | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/website/src/views/components/ScrollToTop.test.tsx b/website/src/views/components/ScrollToTop.test.tsx index b33a2b0a37..040c9d91a0 100644 --- a/website/src/views/components/ScrollToTop.test.tsx +++ b/website/src/views/components/ScrollToTop.test.tsx @@ -1,6 +1,5 @@ -import { act } from 'react-dom/test-utils'; import { Router } from 'react-router-dom'; -import { mount } from 'enzyme'; +import { act, render } from '@testing-library/react'; import createHistory from 'test-utils/createHistory'; import { mockDom, mockDomReset } from 'test-utils/mockDom'; @@ -22,16 +21,14 @@ describe('ScrollToTopComponent', () => { function make(props: Props = {}) { const { history } = createHistory(); - act(() => { - mount( - - - , - ); - }); + render( + + + , + ); return history; } From 6b84a2c426ab5843a654fc11387415173b8c0cd5 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 13:19:03 +0800 Subject: [PATCH 19/25] Fix nits --- .../src/views/components/ScrollToTop.test.tsx | 2 +- .../layout/GlobalSearchContainer.test.tsx | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/website/src/views/components/ScrollToTop.test.tsx b/website/src/views/components/ScrollToTop.test.tsx index 040c9d91a0..55e2e1e565 100644 --- a/website/src/views/components/ScrollToTop.test.tsx +++ b/website/src/views/components/ScrollToTop.test.tsx @@ -1,5 +1,5 @@ -import { Router } from 'react-router-dom'; import { act, render } from '@testing-library/react'; +import { Router } from 'react-router-dom'; import createHistory from 'test-utils/createHistory'; import { mockDom, mockDomReset } from 'test-utils/mockDom'; diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index 6258bf1fe3..599fabc1ed 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -1,25 +1,26 @@ +import type { ModuleCondensed } from 'types/modules'; + import { act, render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import _ from 'lodash'; import { Provider } from 'react-redux'; import { MemoryRouter } from 'react-router-dom'; import produce from 'immer'; +import { range } from 'lodash'; import configureStore from 'bootstrapping/configure-store'; import reducers from 'reducers'; -import type { ModuleCondensed } from 'types/modules'; -import { fetchVenueList } from 'actions/venueBank'; -import GlobalSearchContainer from 'views/layout/GlobalSearchContainer'; import { initAction } from 'test-utils/redux'; import { mockDom, mockDomReset, mockWindowMatchMedia } from 'test-utils/mockDom'; -jest.mock('actions/venueBank'); +import GlobalSearchContainer from 'views/layout/GlobalSearchContainer'; +import { fetchVenueList } from 'actions/venueBank'; +jest.mock('actions/venueBank'); const mockedFetchVenueList = fetchVenueList as jest.MockedFunction; const letters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'.split(''); // Produces 26 * 26 = 676 modules of the form AA1010, AB1010, ... -const MODULES = _.flatMap(letters, (firstLetter): ModuleCondensed[] => +const MODULES: ModuleCondensed[] = letters.flatMap((firstLetter) => letters.map((secondLetter) => ({ moduleCode: `${firstLetter}${secondLetter}1010`, title: 'Test', @@ -40,8 +41,8 @@ function make(storeOverrides: Partial = {}) { produce(reducers(undefined, initAction()), (draft) => { draft.moduleBank.moduleList = (storeOverrides.moduleBank?.moduleList ?? relevantStoreContents.moduleBank.moduleList) as typeof draft.moduleBank.moduleList; - draft.venueBank.venueList = (storeOverrides.venueBank?.venueList ?? - relevantStoreContents.venueBank.venueList) as typeof draft.venueBank.venueList; + draft.venueBank.venueList = + storeOverrides.venueBank?.venueList ?? relevantStoreContents.venueBank.venueList; }), ); @@ -197,7 +198,7 @@ describe(GlobalSearchContainer, () => { test('show many results if the search only returns results of one type', () => { const { getByRole, getAllByRole } = make({ - venueBank: { venueList: _.range(100).map((n) => `Venue ${n}`) }, + venueBank: { venueList: range(100).map((n) => `Venue ${n}`) }, }); userEvent.type(getByRole('textbox'), '1010'); From b6757117269e0a7fcbb00c95b21065af5318e734 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 15:39:30 +0800 Subject: [PATCH 20/25] Split GlobalSearchContainer show many results test --- website/src/views/layout/GlobalSearchContainer.test.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index 599fabc1ed..5be357a181 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -196,7 +196,7 @@ describe(GlobalSearchContainer, () => { `); }); - test('show many results if the search only returns results of one type', () => { + test('show many results if the search only returns modules', () => { const { getByRole, getAllByRole } = make({ venueBank: { venueList: range(100).map((n) => `Venue ${n}`) }, }); @@ -277,8 +277,12 @@ describe(GlobalSearchContainer, () => { "CR1010 TestSem 1", ] `); + }); - userEvent.clear(getByRole('textbox')); + test('show many results if the search only returns venues', () => { + const { getByRole, getAllByRole } = make({ + venueBank: { venueList: range(100).map((n) => `Venue ${n}`) }, + }); userEvent.type(getByRole('textbox'), 'venue'); expect(getAllByRole('option').map((elem) => elem.textContent)).toMatchInlineSnapshot(` From 02e91a6e58e13e30501d74df0406a30417ed0e94 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 17:28:36 +0800 Subject: [PATCH 21/25] Fix review comments --- website/src/utils/css.ts | 9 ++++----- website/src/views/components/KeyboardShortcuts.tsx | 2 +- website/src/views/components/RandomKawaii.tsx | 6 ++++-- .../views/components/module-info/LessonTimetable.tsx | 4 ++-- website/src/views/hooks/useMediaQuery.ts | 2 +- .../src/views/layout/GlobalSearchContainer.test.tsx | 11 +++++++---- 6 files changed, 19 insertions(+), 15 deletions(-) diff --git a/website/src/utils/css.ts b/website/src/utils/css.ts index 3a690eefc3..b05c42faa6 100644 --- a/website/src/utils/css.ts +++ b/website/src/utils/css.ts @@ -2,26 +2,25 @@ import type { QueryObject } from 'json2mq'; // NOTE: Keep in sync with Bootstrap's breakpoints. // Breakpoints at time of writing: https://getbootstrap.com/docs/4.5/layout/overview/ -const breakpoints = Object.freeze({ +const breakpoints = { xs: 0, sm: 576, md: 768, lg: 992, xl: 1200, -}); +} as const; type Breakpoint = keyof typeof breakpoints; -function nextBreakpoint(size: Breakpoint): number | null { +function nextBreakpoint(size: Breakpoint): number | undefined { const breakpointEntries = Object.entries(breakpoints); const nextBreakpointIndex = breakpointEntries.findIndex(([breakpoint]) => breakpoint === size) + 1; - if (nextBreakpointIndex >= breakpointEntries.length) return null; return breakpointEntries[nextBreakpointIndex][1]; } export function breakpointDown(size: Breakpoint): QueryObject { const nextSize = nextBreakpoint(size); - if (nextSize === null) return { all: true }; + if (nextSize === undefined) return { all: true }; return { maxWidth: nextSize - 1 }; } diff --git a/website/src/views/components/KeyboardShortcuts.tsx b/website/src/views/components/KeyboardShortcuts.tsx index 6c18d50937..fb5d7c3f6c 100644 --- a/website/src/views/components/KeyboardShortcuts.tsx +++ b/website/src/views/components/KeyboardShortcuts.tsx @@ -127,7 +127,7 @@ const KeyboardShortcuts: React.FC = () => { notifyThemeChange(); }); - bind(['c', '0'], APPEARANCE, 'Next Theme', () => { + bind('c', APPEARANCE, 'Next Theme', () => { dispatch(cycleTheme(1)); notifyThemeChange(); }); diff --git a/website/src/views/components/RandomKawaii.tsx b/website/src/views/components/RandomKawaii.tsx index e2c35f4c6c..454dfa8ba0 100644 --- a/website/src/views/components/RandomKawaii.tsx +++ b/website/src/views/components/RandomKawaii.tsx @@ -8,8 +8,10 @@ const icons = [SpeechBubble, Mug, Browser, Ghost]; const defaultMoods: KawaiiMood[] = ['ko', 'sad', 'shocked']; const RandomKawaii: FC = ({ size, color = '#FF715D', mood, ...wrapperProps }) => { - const [Kawaii] = useState(() => sample(icons) ?? icons[0]); - const [defaultMood] = useState(() => sample(defaultMoods) ?? defaultMoods[0]); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const [Kawaii] = useState(() => sample(icons)!); + const [defaultMood] = useState(() => sample(defaultMoods)); + return (
diff --git a/website/src/views/components/module-info/LessonTimetable.tsx b/website/src/views/components/module-info/LessonTimetable.tsx index 4e72996587..1d9c66a45f 100644 --- a/website/src/views/components/module-info/LessonTimetable.tsx +++ b/website/src/views/components/module-info/LessonTimetable.tsx @@ -1,4 +1,4 @@ -import { FC, useState } from 'react'; +import { FC, memo, useState } from 'react'; import { useHistory } from 'react-router-dom'; import type { SemesterData } from 'types/modules'; @@ -54,4 +54,4 @@ const LessonTimetable: FC<{ allSemesterData: readonly SemesterData[] }> = ({ all ); }; -export default LessonTimetable; +export default memo(LessonTimetable); diff --git a/website/src/views/hooks/useMediaQuery.ts b/website/src/views/hooks/useMediaQuery.ts index fff951a5d3..11a584c0a6 100644 --- a/website/src/views/hooks/useMediaQuery.ts +++ b/website/src/views/hooks/useMediaQuery.ts @@ -26,7 +26,7 @@ export default function useMediaQuery(mediaQuery: string | QueryObject | QueryOb ); // TODO: Replace with useMutableSource after we adopt React concurrent mode. See: - // Docs: https://github.com/bvaughn/rfcs/blob/useMutableSource/text/0000-use-mutable-source.md + // Docs: https://github.com/reactjs/rfcs/blob/master/text/0147-use-mutable-source.md // Example: https://github.com/facebook/react/blob/36df483/packages/react-devtools-scheduling-profiler/src/hooks.js const matchedBreakpoint = useSubscription(subscription); diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index 5be357a181..e636b3934b 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -1,4 +1,5 @@ -import type { ModuleCondensed } from 'types/modules'; +import type { ModuleList } from 'types/reducers'; +import type { VenueList } from 'types/venues'; import { act, render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; @@ -20,7 +21,7 @@ const mockedFetchVenueList = fetchVenueList as jest.MockedFunction +const MODULES: ModuleList = letters.flatMap((firstLetter) => letters.map((secondLetter) => ({ moduleCode: `${firstLetter}${secondLetter}1010`, title: 'Test', @@ -29,16 +30,18 @@ const MODULES: ModuleCondensed[] = letters.flatMap((firstLetter) => ); // Produces 26 venues of the form AA-1, BB-1, CC-1, ... -const VENUES = letters.map((letter) => `${letter}${letter}-1`); +const VENUES: VenueList = letters.map((letter) => `${letter}${letter}-1`); const relevantStoreContents = { moduleBank: { moduleList: MODULES }, venueBank: { venueList: VENUES }, }; +const initialState = reducers(undefined, initAction()); + function make(storeOverrides: Partial = {}) { const { store } = configureStore( - produce(reducers(undefined, initAction()), (draft) => { + produce(initialState, (draft) => { draft.moduleBank.moduleList = (storeOverrides.moduleBank?.moduleList ?? relevantStoreContents.moduleBank.moduleList) as typeof draft.moduleBank.moduleList; draft.venueBank.venueList = From fb67e1f296c36dbbc3e7173bfab0e8e348759a8f Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 18:44:32 +0800 Subject: [PATCH 22/25] Define new, simpler useScrollToTopEffect hook to replace ScrollToTop `ScrollToTop` was used in exactly 1 way: scroll to top/hash on mount. The new hook implements that and removes all configuration and props. --- website/src/utils/react.tsx | 3 +- .../src/views/components/ScrollToTop.test.tsx | 69 --- website/src/views/components/ScrollToTop.tsx | 57 +-- .../ContributeContainer.tsx | 466 +++++++++--------- .../views/hooks/useScrollToTopEffect.test.tsx | 57 +++ .../src/views/hooks/useScrollToTopEffect.ts | 22 + .../src/views/modules/ModulePageContent.tsx | 2 +- .../src/views/settings/SettingsContainer.tsx | 2 +- website/src/views/static/StaticPage.tsx | 23 +- .../views/timetable/TimetableContainer.tsx | 2 +- 10 files changed, 338 insertions(+), 365 deletions(-) delete mode 100644 website/src/views/components/ScrollToTop.test.tsx create mode 100644 website/src/views/hooks/useScrollToTopEffect.test.tsx create mode 100644 website/src/views/hooks/useScrollToTopEffect.ts diff --git a/website/src/utils/react.tsx b/website/src/utils/react.tsx index fa57c6db4a..077287e650 100644 --- a/website/src/utils/react.tsx +++ b/website/src/utils/react.tsx @@ -84,8 +84,7 @@ export function wrapComponentName(Component: React.ComponentType, wrapper: * the component is not loaded on initial page load (ie. the element is not in the DOM when * the page is initially loaded), but has content that can be linked to via hashes. */ -export function scrollToHash() { - const { hash } = window.location; +export function scrollToHash(hash: string) { if (hash) { const ele = document.getElementById(hash.slice(1)); // Hash string contains the '#' character if (ele) { diff --git a/website/src/views/components/ScrollToTop.test.tsx b/website/src/views/components/ScrollToTop.test.tsx deleted file mode 100644 index 55e2e1e565..0000000000 --- a/website/src/views/components/ScrollToTop.test.tsx +++ /dev/null @@ -1,69 +0,0 @@ -import { act, render } from '@testing-library/react'; -import { Router } from 'react-router-dom'; -import createHistory from 'test-utils/createHistory'; -import { mockDom, mockDomReset } from 'test-utils/mockDom'; - -import ScrollToTop from './ScrollToTop'; - -type Props = { - onComponentDidMount?: boolean; - onPathChange?: boolean; -}; - -describe('ScrollToTopComponent', () => { - beforeEach(() => { - mockDom(); - }); - - afterEach(() => { - mockDomReset(); - }); - - function make(props: Props = {}) { - const { history } = createHistory(); - render( - - - , - ); - return history; - } - - test('default behavior does not do anything', () => { - make(); - expect(window.scrollTo).not.toHaveBeenCalled(); - }); - - test('onComponentDidMount attribute behaves correctly', () => { - make({ onComponentDidMount: true }); - expect(window.scrollTo).toHaveBeenCalledWith(0, 0); - }); - - test('onPathChange attribute behaves correctly', () => { - const history = make({ onPathChange: true }); - expect(window.scrollTo).not.toHaveBeenCalled(); - act(() => history.push('/foo')); - expect(window.scrollTo).toHaveBeenCalledWith(0, 0); - }); - - test('onComponentDidMount attribute behaves correctly', () => { - make({ onComponentDidMount: true }); - expect(window.scrollTo).toHaveBeenCalledWith(0, 0); - }); - - test('integration test', () => { - const history = make({ onComponentDidMount: true, onPathChange: true }); - expect(window.scrollTo).toHaveBeenCalledTimes(1); - expect(window.scrollTo).toHaveBeenCalledWith(0, 0); - act(() => history.push('/foo')); - expect(window.scrollTo).toHaveBeenCalledTimes(2); - expect(window.scrollTo).toHaveBeenCalledWith(0, 0); - act(() => history.push('/foo')); - expect(window.scrollTo).toHaveBeenCalledTimes(2); - act(() => history.push('/bar')); - expect(window.scrollTo).toHaveBeenCalledTimes(3); - }); -}); diff --git a/website/src/views/components/ScrollToTop.tsx b/website/src/views/components/ScrollToTop.tsx index 710c4fb3e8..a939375237 100644 --- a/website/src/views/components/ScrollToTop.tsx +++ b/website/src/views/components/ScrollToTop.tsx @@ -1,52 +1,11 @@ -import { useEffect, useRef } from 'react'; -import type { LocationKey } from 'history'; -import { useLocation } from 'react-router-dom'; -import { scrollToHash } from 'utils/react'; - -function scrollToTop() { - window.scrollTo(0, 0); -} - -export type Props = { - onComponentDidMount?: boolean; - onPathChange?: boolean; - shouldScrollToHash?: boolean; -}; - -const ScrollToTop: React.FC = ({ - onComponentDidMount = false, - onPathChange = false, - shouldScrollToHash = true, -}) => { - useEffect( - () => { - if (onComponentDidMount && !window.location.hash) { - scrollToTop(); - } else if (shouldScrollToHash) { - scrollToHash(); - } - }, - // This effect should only be run on component mount; don't care if props - // change afterwards. - // eslint-disable-next-line react-hooks/exhaustive-deps - [], - ); - - const previousLocationKey = useRef(null); - const location = useLocation(); - useEffect(() => { - if ( - onPathChange && - // Don't scroll to top on initial mount (i.e. when previousLocationKey.current is null) - previousLocationKey.current && - // Don't scroll to top if no navigation has happened - previousLocationKey.current !== location.pathname - ) { - scrollToTop(); - } - previousLocationKey.current = location.pathname; - }, [onPathChange, location.pathname]); - +import type { FC } from 'react'; +import useScrollToTopEffect from 'views/hooks/useScrollToTopEffect'; + +/** + * @deprecated Use `useScrollToTopEffect` instead + */ +const ScrollToTop: FC = () => { + useScrollToTopEffect(); return null; }; diff --git a/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx b/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx index aedd416239..51628194c9 100644 --- a/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx +++ b/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx @@ -1,20 +1,21 @@ -import { memo, Fragment } from 'react'; +import { memo, Fragment, FC } from 'react'; import { connect } from 'react-redux'; import classnames from 'classnames'; import { Link } from 'react-router-dom'; import { flatten, map, mapValues, values } from 'lodash'; -import { ModuleCondensed } from 'types/modules'; +import type { ModuleCondensed } from 'types/modules'; +import type { State as StoreState } from 'types/state'; import { toggleFeedback } from 'actions/app'; import { toggleBetaTesting } from 'actions/settings'; import { modulePage } from 'views/routes/paths'; import { DollarSign, Zap } from 'react-feather'; import ExternalLink from 'views/components/ExternalLink'; -import ScrollToTop from 'views/components/ScrollToTop'; import Title from 'views/components/Title'; import { FeedbackButtons } from 'views/components/FeedbackModal'; import { getModuleCondensed } from 'selectors/moduleBank'; +import useScrollToTopEffect from 'views/hooks/useScrollToTopEffect'; import { currentTests } from 'views/settings/BetaToggle'; import { notNull } from 'types/utils'; import config from 'config'; @@ -27,7 +28,6 @@ import BugReportIcon from 'img/icons/bug-report.svg'; import DeveloperIcon from 'img/icons/programmer.svg'; import ContributeIcon from 'img/icons/love.svg'; import VenueIcon from 'img/icons/compass.svg'; -import { State as StoreState } from 'types/state'; import UnmappedVenues from '../UnmappedVenues'; import ContributorList from '../ContributorList'; @@ -43,272 +43,276 @@ type Props = { toggleBetaTesting: () => void; }; -const ContributeContainer = memo(({ modules, beta, ...props }) => ( -
- - Contribute +const ContributeContainer: FC = ({ modules, beta, ...props }) => { + useScrollToTopEffect(); + return ( +
+ Contribute -
- -

Help Us Help You!

-
- -

- NUSMods is a 100% student-run, open source project. We rely on the continuous support of our - contributors and the NUS student community. Many students have reported issues, suggested - improvements, and even contributed code. Join us to make NUS a better place for its students - and your friends! -

- -
-

For Everyone

-
- - {flatten(values(modules)).length > 0 && ( -
-
- -

Write Module Reviews

-
- -

- Help your fellow students make better choices when planning their module by leaving your - honest opinions on modules you have taken before. Here are all of the modules you have - taken this year: -

- -
- {map(modules, (moduleCondensed, semester) => ( - -

{config.semesterNames[semester]}

-
- {moduleCondensed.map(({ moduleCode, title }) => ( - - Review {moduleCode} {title} - - ))} -
- - ))} -
-
- )} - -
- -

Map the School

+ +

Help Us Help You!

- We are mapping venues found on timetables to help students get around the school. This is - especially useful for freshmen and exchange students who find NUS hard to navigate. + NUSMods is a 100% student-run, open source project. We rely on the continuous support of our + contributors and the NUS student community. Many students have reported issues, suggested + improvements, and even contributed code. Join us to make NUS a better place for its students + and your friends!

- -
+
+

For Everyone

+
+ + {flatten(values(modules)).length > 0 && ( +
+
+ +

Write Module Reviews

+
+ +

+ Help your fellow students make better choices when planning their module by leaving your + honest opinions on modules you have taken before. Here are all of the modules you have + taken this year: +

+ +
+ {map(modules, (moduleCondensed, semester) => ( + +

{config.semesterNames[semester]}

+
+ {moduleCondensed.map(({ moduleCode, title }) => ( + + Review {moduleCode} {title} + + ))} +
+ + ))} +
+
+ )} - {currentTests.length > 0 && (
- -

Test Drive NUSMods Beta

+ +

Map the School

- We're constantly updating NUSMods with new and exciting features, and you can use - them before everyone else by participating in NUSMods Beta. Help find bugs and provide - feedback to shape the future of NUSMods. Currently we are testing the following features: + We are mapping venues found on timetables to help students get around the school. This is + especially useful for freshmen and exchange students who find NUS hard to navigate.

-
    - {currentTests.map((test, index) => ( -
  • {test}
    • - ))} -
+ +
+ + {currentTests.length > 0 && ( +
+
+ +

Test Drive NUSMods Beta

+
+ +

+ We're constantly updating NUSMods with new and exciting features, and you can use + them before everyone else by participating in NUSMods Beta. Help find bugs and provide + feedback to shape the future of NUSMods. Currently we are testing the following + features: +

- {beta ? ( - <> -

You are already in the beta program.

+
    + {currentTests.map((test, index) => ( +
  • {test}
    • + ))} +
+ + {beta ? ( + <> +

You are already in the beta program.

+

+ +

+

+ Go to settings if you wish to stop using NUSMods + Beta. +

+ + ) : (

-

- Go to settings if you wish to stop using NUSMods - Beta. -

- - ) : ( -

- -

- )} -
- )} - -
-
- -

Give Us Feedback

-
+ )} +
+ )} -

- We are always open to feedback. If you see something that doesn't seem quite right, or - have a new idea for making NUSMods better, you can use these links below to reach us. -

+
+
+ +

Give Us Feedback

+
- -
+

+ We are always open to feedback. If you see something that doesn't seem quite right, + or have a new idea for making NUSMods better, you can use these links below to reach us. +

-
-
- -

Donate

-
-

- NUSMods servers are currently graciously paid for by the school, but we still need money to - promote NUSMods and find contributors to ensure the site is maintained in the long term. - More contributors will also mean we can work on bringing you new features, such as those - currently in beta. Your contributions will go towards the cost of promotional materials such - as T-shirts and stickers. Our expenses are transparent and can be viewed on{' '} - OpenCollective. -

+ +
-

These are our current backers:

+
+
+ +

Donate

+
+

+ NUSMods servers are currently graciously paid for by the school, but we still need money + to promote NUSMods and find contributors to ensure the site is maintained in the long + term. More contributors will also mean we can work on bringing you new features, such as + those currently in beta. Your contributions will go towards the cost of promotional + materials such as T-shirts and stickers. Our expenses are transparent and can be viewed on{' '} + OpenCollective. +

-

- - Avatar of our backers - -

+

These are our current backers:

-

- - - Donate to NUSMods - -

-
+

+ + Avatar of our backers + +

-
-

For Developers & Designers

-
+

+ + + Donate to NUSMods + +

+
-
- -

File Bug Reports and Feature Requests

+

For Developers & Designers

-

- If you have an account on GitHub, you can file bug reports and feature request directly - using GitHub issues. -

+
+
+ +

File Bug Reports and Feature Requests

+
-
- -

Bug Report

-

Create a report to help reproduce and fix the issue

- - -

Feature Request

-

Suggest a new feature or enhancement for the project

- -
-
- -
-
- -

Contribute Code and Design

-
+

+ If you have an account on GitHub, you can file bug reports and feature request directly + using GitHub issues. +

-

- You can also help directly contribute code and design. We welcome all contributions, big and - small. To get your feet wet, we suggest starting with the good first issues suitable for - first time contributors of various skill levels. We think NUSMods is a good way to learn - modern web development on a production web application and make a positive impact on the - lives of NUS students. -

+
+ +

Bug Report

+

Create a report to help reproduce and fix the issue

+ + +

Feature Request

+

Suggest a new feature or enhancement for the project

+ +
+
-
- -

Good First Issues

-

Issues with limited scope good for first time contributors

- - -

Contribution Guide

-

Information for first time contributors

- - -

Telegram Chat

-

Talk to us about NUSMods design and development

- - -

Mailing List

-

Subscribe to news and updates

- -
+
+
+ +

Contribute Code and Design

+
-

- Here are our top NUSMods contributors. Previous maintainers have gone on to work at Google, - Facebook, and other prestigious technology companies. You could be next! -

+

+ You can also help directly contribute code and design. We welcome all contributions, big + and small. To get your feet wet, we suggest starting with the good first issues suitable + for first time contributors of various skill levels. We think NUSMods is a good way to + learn modern web development on a production web application and make a positive impact on + the lives of NUS students. +

- +
+ +

Good First Issues

+

Issues with limited scope good for first time contributors

+ + +

Contribution Guide

+

Information for first time contributors

+ + +

Telegram Chat

+

Talk to us about NUSMods design and development

+ + +

Mailing List

+

Subscribe to news and updates

+ +
-

- - View all contributors → - -

-
+

+ Here are our top NUSMods contributors. Previous maintainers have gone on to work at + Google, Facebook, and other prestigious technology companies. You could + be next! +

-

- Icon made by Freepik from{' '} - www.flaticon.com -

- -)); + + +

+ + View all contributors → + +

+
+ +

+ Icon made by Freepik from{' '} + www.flaticon.com +

+ + ); +}; const ConnectedContributeContainer = connect( (state: StoreState) => { @@ -324,6 +328,6 @@ const ConnectedContributeContainer = connect( }; }, { toggleFeedback, toggleBetaTesting }, -)(ContributeContainer); +)(memo(ContributeContainer)); export default ConnectedContributeContainer; diff --git a/website/src/views/hooks/useScrollToTopEffect.test.tsx b/website/src/views/hooks/useScrollToTopEffect.test.tsx new file mode 100644 index 0000000000..6d3dce81ff --- /dev/null +++ b/website/src/views/hooks/useScrollToTopEffect.test.tsx @@ -0,0 +1,57 @@ +import { act, render } from '@testing-library/react'; +import type { FC } from 'react'; +import { Router } from 'react-router-dom'; +import createHistory from 'test-utils/createHistory'; +import { mockDom, mockDomReset } from 'test-utils/mockDom'; +import { scrollToHash } from 'utils/react'; + +import useScrollToTopEffect from './useScrollToTopEffect'; + +jest.mock('utils/react'); +const mockedScrollToHash = scrollToHash as jest.MockedFunction; + +const Tester: FC = () => { + useScrollToTopEffect(); + return null; +}; + +describe(useScrollToTopEffect, () => { + beforeEach(() => { + mockDom(); + }); + + afterEach(() => { + mockedScrollToHash.mockReset(); + mockDomReset(); + }); + + function make(initialHistoryEntries: Parameters[0] = undefined) { + const { history } = createHistory(initialHistoryEntries); + render( + + + , + ); + return history; + } + + test('should scroll to top on mount if window location has no hash', () => { + make(); + expect(window.scrollTo).toHaveBeenCalledWith(0, 0); + expect(mockedScrollToHash).not.toHaveBeenCalled(); + }); + + test('should scroll to hash on mount if window location has a hash', () => { + make('/foo#hash'); + expect(window.scrollTo).not.toHaveBeenCalled(); + expect(mockedScrollToHash).toHaveBeenCalled(); + }); + + test('should not scroll after first completed render', () => { + const history = make(); + expect(window.scrollTo).toHaveBeenCalledTimes(1); // Sanity check + act(() => history.push('/bar')); + act(() => history.push('/baz#hash')); + expect(window.scrollTo).toHaveBeenCalledTimes(1); + }); +}); diff --git a/website/src/views/hooks/useScrollToTopEffect.ts b/website/src/views/hooks/useScrollToTopEffect.ts new file mode 100644 index 0000000000..cf88796551 --- /dev/null +++ b/website/src/views/hooks/useScrollToTopEffect.ts @@ -0,0 +1,22 @@ +import { useEffect } from 'react'; +import { useLocation } from 'react-router-dom'; +import { scrollToHash } from 'utils/react'; + +function scrollToTop() { + window.scrollTo(0, 0); +} + +/** + * Scrolls to top (or hash, if a hash is present in the URL) on the first + * completed render. + */ +export default function useScrollToTopEffect() { + // TODO: Prevent location changes from triggering renders, as we don't care. + // We can't use window.location to support non-browser environments (e.g. + // tests). + const { hash } = useLocation(); + + // Don't care if anything changes after the first render. + // eslint-disable-next-line react-hooks/exhaustive-deps + useEffect(() => (hash ? scrollToHash(hash) : scrollToTop()), []); +} diff --git a/website/src/views/modules/ModulePageContent.tsx b/website/src/views/modules/ModulePageContent.tsx index 38c37ac123..5b71dee33c 100644 --- a/website/src/views/modules/ModulePageContent.tsx +++ b/website/src/views/modules/ModulePageContent.tsx @@ -81,7 +81,7 @@ class ModulePageContent extends Component { - + {isArchive && (
diff --git a/website/src/views/settings/SettingsContainer.tsx b/website/src/views/settings/SettingsContainer.tsx index c4cda2c9fe..012a4ede82 100644 --- a/website/src/views/settings/SettingsContainer.tsx +++ b/website/src/views/settings/SettingsContainer.tsx @@ -158,7 +158,7 @@ class SettingsContainer extends Component { return (
- + Settings diff --git a/website/src/views/static/StaticPage.tsx b/website/src/views/static/StaticPage.tsx index 1f58e4a341..beecd50ee2 100644 --- a/website/src/views/static/StaticPage.tsx +++ b/website/src/views/static/StaticPage.tsx @@ -1,23 +1,24 @@ -import * as React from 'react'; +import type { FC } from 'react'; import classnames from 'classnames'; -import ScrollToTop from 'views/components/ScrollToTop'; import Title from 'views/components/Title'; +import useScrollToTopEffect from 'views/hooks/useScrollToTopEffect'; type Props = { title: string; - children: React.ReactNode; className?: string; }; -const StaticPage: React.FC = (props) => ( -
- - {props.title} -
-
{props.children}
+const StaticPage: FC = ({ title, className, children }) => { + useScrollToTopEffect(); + return ( +
+ {title} +
+
{children}
+
-
-); + ); +}; export default StaticPage; diff --git a/website/src/views/timetable/TimetableContainer.tsx b/website/src/views/timetable/TimetableContainer.tsx index e050b329de..14c8a89d65 100644 --- a/website/src/views/timetable/TimetableContainer.tsx +++ b/website/src/views/timetable/TimetableContainer.tsx @@ -207,7 +207,7 @@ export class TimetableContainerComponent extends PureComponent { return (
- + Date: Wed, 2 Dec 2020 18:51:19 +0800 Subject: [PATCH 23/25] Nit: clarify useScrollToTopEffect useEffect comment --- website/src/views/hooks/useScrollToTopEffect.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/src/views/hooks/useScrollToTopEffect.ts b/website/src/views/hooks/useScrollToTopEffect.ts index cf88796551..37d9c0c8ba 100644 --- a/website/src/views/hooks/useScrollToTopEffect.ts +++ b/website/src/views/hooks/useScrollToTopEffect.ts @@ -16,7 +16,7 @@ export default function useScrollToTopEffect() { // tests). const { hash } = useLocation(); - // Don't care if anything changes after the first render. + // Don't do anything after the first completed render. // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => (hash ? scrollToHash(hash) : scrollToTop()), []); } From 8b0cf987987182a588595d944f8d76bf1e9abea4 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Wed, 2 Dec 2020 18:54:38 +0800 Subject: [PATCH 24/25] Nit: further clarify useScrollToTopEffect comments --- website/src/views/hooks/useScrollToTopEffect.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/website/src/views/hooks/useScrollToTopEffect.ts b/website/src/views/hooks/useScrollToTopEffect.ts index 37d9c0c8ba..dba1682712 100644 --- a/website/src/views/hooks/useScrollToTopEffect.ts +++ b/website/src/views/hooks/useScrollToTopEffect.ts @@ -7,7 +7,7 @@ function scrollToTop() { } /** - * Scrolls to top (or hash, if a hash is present in the URL) on the first + * Scrolls to top (or hash, if a hash is present in the URL) after the first * completed render. */ export default function useScrollToTopEffect() { @@ -16,7 +16,7 @@ export default function useScrollToTopEffect() { // tests). const { hash } = useLocation(); - // Don't do anything after the first completed render. + // Intentionally *not* care after executing once. // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => (hash ? scrollToHash(hash) : scrollToTop()), []); } From f019ae1445d24496e1113d56faadf8b79944fa00 Mon Sep 17 00:00:00 2001 From: E-Liang Tan Date: Thu, 3 Dec 2020 11:53:48 +0800 Subject: [PATCH 25/25] Fix review comments --- website/src/views/components/RandomKawaii.tsx | 4 ++-- website/src/views/components/ScrollToTop.tsx | 6 +++--- .../contribute/ContributeContainer/ContributeContainer.tsx | 4 ++-- ...seScrollToTopEffect.test.tsx => useScrollToTop.test.tsx} | 6 +++--- .../hooks/{useScrollToTopEffect.ts => useScrollToTop.ts} | 2 +- website/src/views/layout/GlobalSearchContainer.test.tsx | 2 +- website/src/views/layout/GlobalSearchContainer.tsx | 4 ++-- website/src/views/static/StaticPage.tsx | 4 ++-- 8 files changed, 16 insertions(+), 16 deletions(-) rename website/src/views/hooks/{useScrollToTopEffect.test.tsx => useScrollToTop.test.tsx} (92%) rename website/src/views/hooks/{useScrollToTopEffect.ts => useScrollToTop.ts} (93%) diff --git a/website/src/views/components/RandomKawaii.tsx b/website/src/views/components/RandomKawaii.tsx index 454dfa8ba0..e633c9fab5 100644 --- a/website/src/views/components/RandomKawaii.tsx +++ b/website/src/views/components/RandomKawaii.tsx @@ -1,4 +1,4 @@ -import { FC, HTMLAttributes, useState } from 'react'; +import { FC, HTMLAttributes, memo, useState } from 'react'; import { sample } from 'lodash'; import { SpeechBubble, Mug, Browser, Ghost, KawaiiMood, KawaiiProps } from 'react-kawaii'; @@ -19,4 +19,4 @@ const RandomKawaii: FC = ({ size, color = '#FF715D', mood, ...wrapperProp ); }; -export default RandomKawaii; +export default memo(RandomKawaii); diff --git a/website/src/views/components/ScrollToTop.tsx b/website/src/views/components/ScrollToTop.tsx index a939375237..e8df1985ae 100644 --- a/website/src/views/components/ScrollToTop.tsx +++ b/website/src/views/components/ScrollToTop.tsx @@ -1,11 +1,11 @@ import type { FC } from 'react'; -import useScrollToTopEffect from 'views/hooks/useScrollToTopEffect'; +import useScrollToTop from 'views/hooks/useScrollToTop'; /** - * @deprecated Use `useScrollToTopEffect` instead + * @deprecated Use `useScrollToTop` instead */ const ScrollToTop: FC = () => { - useScrollToTopEffect(); + useScrollToTop(); return null; }; diff --git a/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx b/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx index 51628194c9..71457187c9 100644 --- a/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx +++ b/website/src/views/contribute/ContributeContainer/ContributeContainer.tsx @@ -15,7 +15,7 @@ import ExternalLink from 'views/components/ExternalLink'; import Title from 'views/components/Title'; import { FeedbackButtons } from 'views/components/FeedbackModal'; import { getModuleCondensed } from 'selectors/moduleBank'; -import useScrollToTopEffect from 'views/hooks/useScrollToTopEffect'; +import useScrollToTop from 'views/hooks/useScrollToTop'; import { currentTests } from 'views/settings/BetaToggle'; import { notNull } from 'types/utils'; import config from 'config'; @@ -44,7 +44,7 @@ type Props = { }; const ContributeContainer: FC = ({ modules, beta, ...props }) => { - useScrollToTopEffect(); + useScrollToTop(); return (
Contribute diff --git a/website/src/views/hooks/useScrollToTopEffect.test.tsx b/website/src/views/hooks/useScrollToTop.test.tsx similarity index 92% rename from website/src/views/hooks/useScrollToTopEffect.test.tsx rename to website/src/views/hooks/useScrollToTop.test.tsx index 6d3dce81ff..5593321f5e 100644 --- a/website/src/views/hooks/useScrollToTopEffect.test.tsx +++ b/website/src/views/hooks/useScrollToTop.test.tsx @@ -5,17 +5,17 @@ import createHistory from 'test-utils/createHistory'; import { mockDom, mockDomReset } from 'test-utils/mockDom'; import { scrollToHash } from 'utils/react'; -import useScrollToTopEffect from './useScrollToTopEffect'; +import useScrollToTop from './useScrollToTop'; jest.mock('utils/react'); const mockedScrollToHash = scrollToHash as jest.MockedFunction; const Tester: FC = () => { - useScrollToTopEffect(); + useScrollToTop(); return null; }; -describe(useScrollToTopEffect, () => { +describe(useScrollToTop, () => { beforeEach(() => { mockDom(); }); diff --git a/website/src/views/hooks/useScrollToTopEffect.ts b/website/src/views/hooks/useScrollToTop.ts similarity index 93% rename from website/src/views/hooks/useScrollToTopEffect.ts rename to website/src/views/hooks/useScrollToTop.ts index dba1682712..10ba79b0d3 100644 --- a/website/src/views/hooks/useScrollToTopEffect.ts +++ b/website/src/views/hooks/useScrollToTop.ts @@ -10,7 +10,7 @@ function scrollToTop() { * Scrolls to top (or hash, if a hash is present in the URL) after the first * completed render. */ -export default function useScrollToTopEffect() { +export default function useScrollToTop() { // TODO: Prevent location changes from triggering renders, as we don't care. // We can't use window.location to support non-browser environments (e.g. // tests). diff --git a/website/src/views/layout/GlobalSearchContainer.test.tsx b/website/src/views/layout/GlobalSearchContainer.test.tsx index e636b3934b..a34938a1f2 100644 --- a/website/src/views/layout/GlobalSearchContainer.test.tsx +++ b/website/src/views/layout/GlobalSearchContainer.test.tsx @@ -57,7 +57,7 @@ function make(storeOverrides: Partial = {}) { , ); } -describe(GlobalSearchContainer, () => { +describe('GlobalSearchContainer', () => { beforeEach(() => { mockDom(); diff --git a/website/src/views/layout/GlobalSearchContainer.tsx b/website/src/views/layout/GlobalSearchContainer.tsx index b05fb01133..d43a19693f 100644 --- a/website/src/views/layout/GlobalSearchContainer.tsx +++ b/website/src/views/layout/GlobalSearchContainer.tsx @@ -2,7 +2,7 @@ import type { ModuleCondensed } from 'types/modules'; import type { Venue } from 'types/venues'; import type { State } from 'types/state'; -import { FC, useCallback, useEffect } from 'react'; +import { FC, memo, useCallback, useEffect } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { useHistory } from 'react-router-dom'; @@ -106,4 +106,4 @@ const GlobalSearchContainer: FC = () => { ); }; -export default GlobalSearchContainer; +export default memo(GlobalSearchContainer); diff --git a/website/src/views/static/StaticPage.tsx b/website/src/views/static/StaticPage.tsx index beecd50ee2..cd7db1612c 100644 --- a/website/src/views/static/StaticPage.tsx +++ b/website/src/views/static/StaticPage.tsx @@ -2,7 +2,7 @@ import type { FC } from 'react'; import classnames from 'classnames'; import Title from 'views/components/Title'; -import useScrollToTopEffect from 'views/hooks/useScrollToTopEffect'; +import useScrollToTop from 'views/hooks/useScrollToTop'; type Props = { title: string; @@ -10,7 +10,7 @@ type Props = { }; const StaticPage: FC = ({ title, className, children }) => { - useScrollToTopEffect(); + useScrollToTop(); return (
{title}