Skip to content

Commit

Permalink
Fixed Toast Notification Trigger (#292)
Browse files Browse the repository at this point in the history
### Summary

A Toast notification for un-finalized semesters would be triggered on
every re-render of the HeaderDisplay component due to the lack of
memoization of props to this component. This PR fixes the problem and
eliminates the usage of the deprecated `useLocalStorageNoSync` hook.
  • Loading branch information
samarth52 authored Feb 27, 2024
1 parent 1474927 commit 495bfa6
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 100 deletions.
68 changes: 52 additions & 16 deletions src/components/Header/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React, { useContext, useMemo } from 'react';
import { ScheduleContext, TermsContext } from '../../contexts';
import HeaderDisplay from '../HeaderDisplay';
import useHeaderActionBarProps from '../../hooks/useHeaderActionBarProps';
import { Term } from '../../types';

import './stylesheet.scss';

Expand All @@ -14,6 +15,24 @@ export type HeaderProps = {
captureRef: React.RefObject<HTMLDivElement>;
};

type VersionState = {
type: 'loaded';
currentVersion: string;
allVersionNames: readonly { id: string; name: string }[];
setCurrentVersion: (next: string) => void;
addNewVersion: (name: string, select?: boolean) => string;
deleteVersion: (id: string) => void;
renameVersion: (id: string, newName: string) => void;
cloneVersion: (id: string, newName: string) => void;
};

type TermsState = {
type: 'loaded';
terms: Term[];
currentTerm: string;
onChangeTerm: (next: string) => void;
};

/**
* Renders the top header component with all state/interactivity,
* and includes controls for top-level tab-based navigation.
Expand Down Expand Up @@ -47,6 +66,37 @@ export default function Header({
}, [pinnedCrns, oscar]);

const headerActionBarProps = useHeaderActionBarProps(captureRef);
const termsState = useMemo(
() => ({
type: 'loaded',
terms,
currentTerm: term,
onChangeTerm: setTerm,
}),
[setTerm, term, terms]
) as TermsState;

const versionsState = useMemo(
() => ({
type: 'loaded',
allVersionNames,
currentVersion,
setCurrentVersion,
addNewVersion,
deleteVersion,
renameVersion,
cloneVersion,
}),
[
addNewVersion,
allVersionNames,
cloneVersion,
currentVersion,
deleteVersion,
renameVersion,
setCurrentVersion,
]
) as VersionState;

return (
<HeaderDisplay
Expand All @@ -56,22 +106,8 @@ export default function Header({
onToggleMenu={onToggleMenu}
tabs={tabs}
{...headerActionBarProps}
termsState={{
type: 'loaded',
terms,
currentTerm: term,
onChangeTerm: setTerm,
}}
versionsState={{
type: 'loaded',
allVersionNames,
currentVersion,
setCurrentVersion,
addNewVersion,
deleteVersion,
renameVersion,
cloneVersion,
}}
termsState={termsState}
versionsState={versionsState}
skeleton={false}
/>
);
Expand Down
42 changes: 22 additions & 20 deletions src/components/HeaderDisplay/index.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState } from 'react';
import React, { useEffect, useState } from 'react';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import {
faBars,
Expand All @@ -18,7 +18,7 @@ import HeaderActionBar from '../HeaderActionBar';
import Modal from '../Modal';
import { AccountContextValue } from '../../contexts/account';
import { Term } from '../../types';
import Toast from '../Toast';
import Toast, { notifyToast } from '../Toast';

import './stylesheet.scss';

Expand All @@ -35,6 +35,15 @@ type VersionState =
cloneVersion: (id: string, newName: string) => void;
};

type TermsState =
| { type: 'loading' }
| {
type: 'loaded';
terms: Term[];
currentTerm: string;
onChangeTerm: (next: string) => void;
};

export type HeaderDisplayProps = {
totalCredits?: number | null;
currentTab: number;
Expand All @@ -47,14 +56,7 @@ export type HeaderDisplayProps = {
enableExportCalendar?: boolean;
onDownloadCalendar?: () => void;
enableDownloadCalendar?: boolean;
termsState:
| { type: 'loading' }
| {
type: 'loaded';
terms: Term[];
currentTerm: string;
onChangeTerm: (next: string) => void;
};
termsState: TermsState;
versionsState: VersionState;
accountState: AccountContextValue | { type: 'loading' };
skeleton: boolean;
Expand Down Expand Up @@ -92,17 +94,17 @@ export default function HeaderDisplay({
// (small mobile is < 600 px wide)
const largeMobile = useScreenWidth(LARGE_MOBILE_BREAKPOINT);

// useEffect(() => {
// if (termsState.type === 'loaded' && !skeleton) {
// const termObject = termsState.terms.filter(
// (term) => term.term === termsState.currentTerm
// )[0];
useEffect(() => {
if (termsState.type === 'loaded' && !skeleton) {
const termObject = termsState.terms.filter(
(term) => term.term === termsState.currentTerm
)[0];

// if (!termObject?.finalized) {
// notifyToast('finalized-term-toast');
// }
// }
// });
if (!termObject?.finalized) {
notifyToast('finalized-term-toast');
}
}
}, [termsState, skeleton]);

return (
<div className="Header">
Expand Down
10 changes: 6 additions & 4 deletions src/data/hooks/useUIStateFromStorage.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { useCallback } from 'react';

import useLocalStorageNoSync from '../../hooks/useLocalStorageNoSync';
import useLocalStorageState from 'use-local-storage-state';

export const UI_STATE_LOCAL_STORAGE_KEY = 'ui-state';

Expand Down Expand Up @@ -36,9 +35,12 @@ type HookResult = {
* but still have the app resume to the last viewed schedule when opened again.
*/
export default function useUIStateFromStorage(): HookResult {
const [{ currentTerm, versionStates }, setUIState] = useLocalStorageNoSync(
const [{ currentTerm, versionStates }, setUIState] = useLocalStorageState(
UI_STATE_LOCAL_STORAGE_KEY,
defaultUIState
{
defaultValue: defaultUIState,
storageSync: false,
}
);

const setTerm = useCallback(
Expand Down
60 changes: 0 additions & 60 deletions src/hooks/useLocalStorageNoSync.ts

This file was deleted.

0 comments on commit 495bfa6

Please sign in to comment.