From 8be3795139ee2a0c91568d42b55017a921824350 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 5 Mar 2025 15:39:09 +0000 Subject: [PATCH 1/9] Reparameterise obtaining context-dependent theme from tags This makes an implicit assumption that the page theme is the source of truth for the current theme. This may not be the case for deeply nested theme overrides, but unless we start adding e.g. chemistry-themed "blocks" inside physics pages I can't see this structure ever occurring. --- .../elements/layout/SidebarLayout.tsx | 25 ++++++++++--------- src/app/services/pageContext.ts | 7 +++--- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/app/components/elements/layout/SidebarLayout.tsx b/src/app/components/elements/layout/SidebarLayout.tsx index 3644539eb8..17a94a41d8 100644 --- a/src/app/components/elements/layout/SidebarLayout.tsx +++ b/src/app/components/elements/layout/SidebarLayout.tsx @@ -3,13 +3,12 @@ import { Col, ColProps, RowProps, Input, Offcanvas, OffcanvasBody, OffcanvasHead import partition from "lodash/partition"; import classNames from "classnames"; import { AssignmentDTO, ContentSummaryDTO, IsaacConceptPageDTO, QuestionDTO, QuizAttemptDTO, RegisteredUserDTO } from "../../../../IsaacApiTypes"; -import { above, ACCOUNT_TAB, ACCOUNT_TABS, AUDIENCE_DISPLAY_FIELDS, below, BOARD_ORDER_NAMES, BoardCompletions, BoardCreators, BoardLimit, BoardSubjects, BoardViews, confirmThen, determineAudienceViews, filterAssignmentsByStatus, filterAudienceViewsByProperties, getDistinctAssignmentGroups, getDistinctAssignmentSetters, getThemeFromContextAndTags, HUMAN_STAGES, ifKeyIsEnter, isAda, isDefined, siteSpecific, useDeviceSize } from "../../../services"; +import { above, ACCOUNT_TAB, ACCOUNT_TABS, AUDIENCE_DISPLAY_FIELDS, below, BOARD_ORDER_NAMES, BoardCompletions, BoardCreators, BoardLimit, BoardSubjects, BoardViews, confirmThen, determineAudienceViews, filterAssignmentsByStatus, filterAudienceViewsByProperties, getDistinctAssignmentGroups, getDistinctAssignmentSetters, getHumanContext, getThemeFromContextAndTags, HUMAN_STAGES, ifKeyIsEnter, isAda, isDefined, siteSpecific, SiteTheme, useDeviceSize } from "../../../services"; import { StageAndDifficultySummaryIcons } from "../StageAndDifficultySummaryIcons"; import { selectors, useAppSelector } from "../../../state"; import { Link, useHistory } from "react-router-dom"; import { AppGroup, AssignmentBoardOrder, Tag } from "../../../../IsaacAppTypes"; import { AffixButton } from "../AffixButton"; -import { getHumanContext } from "../../../services/pageContext"; import { AssignmentState } from "../../pages/MyAssignments"; import { ShowLoadingQuery } from "../../handlers/ShowLoadingQuery"; import { Spacer } from "../Spacer"; @@ -27,11 +26,12 @@ export const MainContent = (props: ColProps) => { return siteSpecific(, props.children); }; -const QuestionLink = (props: React.HTMLAttributes & {question: QuestionDTO, sidebarRef: RefObject}) => { - const { question, sidebarRef, ...rest } = props; +const QuestionLink = (props: React.HTMLAttributes & {question: QuestionDTO}) => { + const { question, ...rest } = props; + const subject = useAppSelector(selectors.pageContext.subject); const audienceFields = filterAudienceViewsByProperties(determineAudienceViews(question.audience), AUDIENCE_DISPLAY_FIELDS); - return
  • + return
  • @@ -42,10 +42,11 @@ const QuestionLink = (props: React.HTMLAttributes & {question: Qu
    • ; }; -const ConceptLink = (props: React.HTMLAttributes & {concept: IsaacConceptPageDTO, sidebarRef: RefObject}) => { - const { concept, sidebarRef, ...rest } = props; +const ConceptLink = (props: React.HTMLAttributes & {concept: IsaacConceptPageDTO}) => { + const { concept, ...rest } = props; + const subject = useAppSelector(selectors.pageContext.subject); - return
  • + return
  • {concept.title} @@ -139,7 +140,7 @@ export const QuestionSidebar = (props: QuestionSidebarProps) => {
    Related concepts
      - {relatedConcepts.map((concept, i) => )} + {relatedConcepts.map((concept, i) => )}
    } {relatedQuestions && relatedQuestions.length > 0 && <> @@ -148,19 +149,19 @@ export const QuestionSidebar = (props: QuestionSidebarProps) => {
    Related questions
      - {relatedQuestions.map((question, i) => )} + {relatedQuestions.map((question, i) => )}
    : <>
    Related {HUMAN_STAGES[pageContextStage[0]]} questions
      - {relatedQuestionsForContextStage.map((question, i) => )} + {relatedQuestionsForContextStage.map((question, i) => )}
    Related questions for other learning stages
      - {relatedQuestionsForOtherStages.map((question, i) => )} + {relatedQuestionsForOtherStages.map((question, i) => )}
    } diff --git a/src/app/services/pageContext.ts b/src/app/services/pageContext.ts index 0d6fa62c64..af68b0cf58 100644 --- a/src/app/services/pageContext.ts +++ b/src/app/services/pageContext.ts @@ -23,15 +23,14 @@ const filterBySubjects = (tags: (TAG_ID | string)[]): SiteTheme[] => { * * If no subject tags are found, `"neutral"` is returned as a default. * - * @param element - The element from which to find the active context theme. + * @param currentTheme - The current page theme. Find via e.g. `useAppSelector(selectors.pageContext.theme)`. * @param tags - The content object tags in which to search for a subject. * @returns The most relevant theme. */ -export const getThemeFromContextAndTags = (element: React.RefObject, tags: (TAG_ID | string)[]): SiteTheme => { - const currentTheme = element.current?.closest("[data-bs-theme]")?.getAttribute("data-bs-theme") as SiteTheme; +export const getThemeFromContextAndTags = (currentTheme: Subject | undefined, tags: (TAG_ID | string)[]): SiteTheme => { const subjectTags = filterBySubjects(tags); - if (currentTheme !== "neutral" && subjectTags.includes(currentTheme)) { + if (currentTheme && subjectTags.includes(currentTheme)) { return currentTheme; } From c551f8ab3e677aa29d77b958560b878f51b3b71a Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 5 Mar 2025 15:41:29 +0000 Subject: [PATCH 2/9] Use context-dependent theme in themeable ListViews When there are multiple valid themes to choose from, pick the one most relevant to the page context. --- .../components/elements/list-groups/ListView.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/app/components/elements/list-groups/ListView.tsx b/src/app/components/elements/list-groups/ListView.tsx index 8acea6ae69..437095f62c 100644 --- a/src/app/components/elements/list-groups/ListView.tsx +++ b/src/app/components/elements/list-groups/ListView.tsx @@ -2,13 +2,13 @@ import React from "react"; import { AbstractListViewItem, ListViewTagProps } from "./AbstractListViewItem"; import { ShortcutResponse, ViewingContext } from "../../../../IsaacAppTypes"; import { determineAudienceViews } from "../../../services/userViewingContext"; -import { DOCUMENT_TYPE, documentTypePathPrefix, SEARCH_RESULT_TYPE, Subject, TAG_ID, TAG_LEVEL, tags } from "../../../services"; +import { DOCUMENT_TYPE, documentTypePathPrefix, getThemeFromContextAndTags, SEARCH_RESULT_TYPE, Subject, TAG_ID, TAG_LEVEL, tags } from "../../../services"; import { ListGroup, ListGroupItemProps } from "reactstrap"; import { TitleIconProps } from "../PageTitle"; import { AffixButton } from "../AffixButton"; import { QuizSummaryDTO } from "../../../../IsaacApiTypes"; import { Link } from "react-router-dom"; -import { showQuizSettingModal, useAppDispatch } from "../../../state"; +import { selectors, showQuizSettingModal, useAppDispatch, useAppSelector } from "../../../state"; export interface ListViewCardProps extends ListGroupItemProps { item: ShortcutResponse; @@ -33,13 +33,14 @@ export const ListViewCard = ({item, icon, subject, linkTags, ...rest}: ListViewC export const QuestionListViewItem = ({item, ...rest} : {item: ShortcutResponse}) => { const breadcrumb = tags.getByIdsAsHierarchy((item.tags || []) as TAG_ID[]).map(tag => tag.title); const audienceViews: ViewingContext[] = determineAudienceViews(item.audience); - const itemSubject = tags.getSpecifiedTag(TAG_LEVEL.subject, item.tags as TAG_ID[])?.id as Subject; + const pageSubject = useAppSelector(selectors.pageContext.subject); + const itemSubject = getThemeFromContextAndTags(pageSubject, tags.getSubjectTags((item.tags || []) as TAG_ID[]).map(t => t.id)); const url = `/${documentTypePathPrefix[DOCUMENT_TYPE.QUESTION]}/${item.id}`; return { - const itemSubject = tags.getSpecifiedTag(TAG_LEVEL.subject, item.tags as TAG_ID[])?.id as Subject; + const pageSubject = useAppSelector(selectors.pageContext.subject); + const itemSubject = getThemeFromContextAndTags(pageSubject, tags.getSubjectTags((item.tags || []) as TAG_ID[]).map(t => t.id)); const url = `/${documentTypePathPrefix[DOCUMENT_TYPE.CONCEPT]}/${item.id}`; return Date: Fri, 7 Mar 2025 12:18:21 +0000 Subject: [PATCH 3/9] Implement sidebars for both types of concept page --- .../elements/layout/SidebarLayout.tsx | 121 +++++++++++++++--- .../list-groups/AbstractListViewItem.tsx | 2 +- src/app/components/pages/Concepts.tsx | 67 ++++++---- src/scss/phy/checkbox.scss | 8 +- src/scss/phy/list-groups.scss | 4 + 5 files changed, 161 insertions(+), 41 deletions(-) diff --git a/src/app/components/elements/layout/SidebarLayout.tsx b/src/app/components/elements/layout/SidebarLayout.tsx index 9847da7c5f..4e4c9cfc79 100644 --- a/src/app/components/elements/layout/SidebarLayout.tsx +++ b/src/app/components/elements/layout/SidebarLayout.tsx @@ -3,7 +3,7 @@ import { Col, ColProps, RowProps, Input, Offcanvas, OffcanvasBody, OffcanvasHead import partition from "lodash/partition"; import classNames from "classnames"; import { AssignmentDTO, ContentSummaryDTO, IsaacConceptPageDTO, QuestionDTO, QuizAttemptDTO, RegisteredUserDTO } from "../../../../IsaacApiTypes"; -import { above, ACCOUNT_TAB, ACCOUNT_TABS, AUDIENCE_DISPLAY_FIELDS, below, BOARD_ORDER_NAMES, BoardCompletions, BoardCreators, BoardLimit, BoardSubjects, BoardViews, confirmThen, determineAudienceViews, filterAssignmentsByStatus, filterAudienceViewsByProperties, getDistinctAssignmentGroups, getDistinctAssignmentSetters, getHumanContext, getThemeFromContextAndTags, HUMAN_STAGES, ifKeyIsEnter, isAda, isDefined, siteSpecific, useDeviceSize } from "../../../services"; +import { above, ACCOUNT_TAB, ACCOUNT_TABS, AUDIENCE_DISPLAY_FIELDS, below, BOARD_ORDER_NAMES, BoardCompletions, BoardCreators, BoardLimit, BoardSubjects, BoardViews, confirmThen, determineAudienceViews, filterAssignmentsByStatus, filterAudienceViewsByProperties, getDistinctAssignmentGroups, getDistinctAssignmentSetters, getHumanContext, getThemeFromContextAndTags, HUMAN_STAGES, HUMAN_SUBJECTS, ifKeyIsEnter, isAda, isDefined, PHY_NAV_SUBJECTS, siteSpecific, TAG_ID, tags, useDeviceSize } from "../../../services"; import { StageAndDifficultySummaryIcons } from "../StageAndDifficultySummaryIcons"; import { selectors, useAppSelector } from "../../../state"; import { Link, useHistory } from "react-router-dom"; @@ -16,6 +16,7 @@ import { Spacer } from "../Spacer"; import { StyledTabPicker } from "../inputs/StyledTabPicker"; import { GroupSelector } from "../../pages/Groups"; import { QuizRubricButton, SectionProgress } from "../quiz/QuizAttemptComponent"; +import { StyledCheckbox } from "../inputs/StyledCheckbox"; export const SidebarLayout = (props: RowProps) => { const { className, ...rest } = props; @@ -186,36 +187,53 @@ export const ConceptSidebar = (props: QuestionSidebarProps) => { -interface FilterCheckboxProps extends React.HTMLAttributes { +interface FilterCheckboxProps extends React.HTMLAttributes { tag: Tag; conceptFilters: Tag[]; setConceptFilters: React.Dispatch>; tagCounts?: Record; + incompatibleTags?: Tag[]; // tags that are removed when this tag is added + dependentTags?: Tag[]; // tags that are removed when this tag is removed + baseTag?: Tag; // tag to add when all tags are removed + checkboxStyle?: "tab" | "button"; + bsSize?: "sm" | "lg"; } const FilterCheckbox = (props : FilterCheckboxProps) => { - const {tag, conceptFilters, setConceptFilters, tagCounts, ...rest} = props; + const {tag, conceptFilters, setConceptFilters, tagCounts, checkboxStyle, incompatibleTags, dependentTags, baseTag, ...rest} = props; const [checked, setChecked] = useState(conceptFilters.includes(tag)); useEffect(() => { setChecked(conceptFilters.includes(tag)); }, [conceptFilters, tag]); - return ) => setConceptFilters(f => e.target.checked ? [...f, tag] : f.filter(c => c !== tag))} - checkboxTitle={tag.title} count={tagCounts && isDefined(tagCounts[tag.id]) ? tagCounts[tag.id] : undefined} - />; + const handleCheckboxChange = (checked: boolean) => { + const newConceptFilters = checked + ? [...conceptFilters.filter(c => !incompatibleTags?.includes(c)), tag] + : conceptFilters.filter(c => ![tag, ...(dependentTags ?? [])].includes(c)); + setConceptFilters(newConceptFilters.length > 0 ? newConceptFilters : (baseTag ? [baseTag] : [])); + }; + + return checkboxStyle === "button" + ? ) => handleCheckboxChange(e.target.checked)} + label={{tag.title} {tagCounts && isDefined(tagCounts[tag.id]) && ({tagCounts[tag.id]})}} + /> + : ) => handleCheckboxChange(e.target.checked)} + checkboxTitle={tag.title} count={tagCounts && isDefined(tagCounts[tag.id]) ? tagCounts[tag.id] : undefined} + />; }; const AllFiltersCheckbox = (props: Omit) => { - const { conceptFilters, setConceptFilters, tagCounts, ...rest } = props; - const [previousFilters, setPreviousFilters] = useState([]); + const { conceptFilters, setConceptFilters, tagCounts, baseTag, ...rest } = props; + const [previousFilters, setPreviousFilters] = useState(baseTag ? [baseTag] : []); return a + b, 0)} + id="all" checked={baseTag ? conceptFilters.length === 1 && conceptFilters[0] === baseTag : !conceptFilters.length} checkboxTitle="All" count={tagCounts && Object.values(tagCounts).reduce((a, b) => a + b, 0)} onInputChange={(e) => { if (e.target.checked) { setPreviousFilters(conceptFilters); - setConceptFilters([]); + setConceptFilters(baseTag ? [baseTag] : []); } else { setConceptFilters(previousFilters); } @@ -237,6 +255,8 @@ export const SubjectSpecificConceptListSidebar = (props: ConceptListSidebarProps const pageContext = useAppSelector(selectors.pageContext.context); + const subjectTag = tags.getById(pageContext?.subject as TAG_ID); + return
    Search concepts
    @@ -251,9 +271,19 @@ export const SubjectSpecificConceptListSidebar = (props: ConceptListSidebarProps
    Filter by topic
    - +
    - {applicableTags.map(tag => )} + {applicableTags.map(tag => + + )}
    @@ -272,9 +302,68 @@ export const SubjectSpecificConceptListSidebar = (props: ConceptListSidebarProps ; }; -export const GenericConceptsSidebar = (props: SidebarProps) => { - // TODO - return ; +export const GenericConceptsSidebar = (props: ConceptListSidebarProps) => { + const { searchText, setSearchText, conceptFilters, setConceptFilters, applicableTags, tagCounts, ...rest } = props; + + const pageContext = useAppSelector(selectors.pageContext.context); + + return +
    +
    Search concepts
    + ) => setSearchText(e.target.value)} + /> + +
    + +
    +
    Filter by subject
    + {Object.keys(PHY_NAV_SUBJECTS).map((subject, i) => { + const subjectTag = tags.getById(subject as TAG_ID); + const descendentTags = tags.getDirectDescendents(subjectTag.id); + const isSelected = conceptFilters.includes(subjectTag) || descendentTags.some(tag => conceptFilters.includes(tag)); + const isPartial = descendentTags.some(tag => conceptFilters.includes(tag)) && descendentTags.some(tag => !conceptFilters.includes(tag)); + return
    + + {isSelected &&
    + {descendentTags + .filter(tag => !isDefined(tagCounts) || tagCounts[tag.id] > 0) + // .sort((a, b) => tagCounts ? tagCounts[b.id] - tagCounts[a.id] : 0) + .map((tag, j) => ) + } +
    } +
    ; + })} +
    + +
    + + {pageContext?.subject && <> +
    + +
    +

    The concepts shown on this page have been filtered to only show those that are relevant to {getHumanContext(pageContext)}.

    +

    If you want to explore broader concepts across multiple subjects or learning stages, you can use the main concept browser:

    + + Browse concepts + +
    + } + ; }; interface QuestionFinderSidebarProps extends SidebarProps { diff --git a/src/app/components/elements/list-groups/AbstractListViewItem.tsx b/src/app/components/elements/list-groups/AbstractListViewItem.tsx index 7b2ec146c2..414125c6e4 100644 --- a/src/app/components/elements/list-groups/AbstractListViewItem.tsx +++ b/src/app/components/elements/list-groups/AbstractListViewItem.tsx @@ -87,7 +87,7 @@ export const AbstractListViewItem = ({icon, title, subject, subtitle, breadcrumb fullWidth = fullWidth || below["sm"](deviceSize) || ((status || audienceViews || previewQuizUrl || quizButton) ? false : true); const colWidths = fullWidth ? [12,12,12,12,12] : isQuiz ? [12,6,6,6,6] : [12,8,7,6,7]; const cardBody = - +
    {icon && ( diff --git a/src/app/components/pages/Concepts.tsx b/src/app/components/pages/Concepts.tsx index 3cce526181..a396d064b3 100644 --- a/src/app/components/pages/Concepts.tsx +++ b/src/app/components/pages/Concepts.tsx @@ -4,7 +4,7 @@ import {AppState, fetchConcepts, selectors, useAppDispatch, useAppSelector} from import {Badge, Card, CardBody, CardHeader, Container} from "reactstrap"; import queryString from "query-string"; import {ShowLoading} from "../handlers/ShowLoading"; -import {isPhy, matchesAllWordsInAnyOrder, pushConceptsToHistory, searchResultIsPublic, shortcuts, TAG_ID, tags} from "../../services"; +import {isAda, isPhy, matchesAllWordsInAnyOrder, pushConceptsToHistory, searchResultIsPublic, shortcuts, TAG_ID, tags} from "../../services"; import {generateSubjectLandingPageCrumbFromContext, TitleAndBreadcrumb} from "../elements/TitleAndBreadcrumb"; import {ShortcutResponse, Tag} from "../../../IsaacAppTypes"; import {IsaacSpinner} from "../handlers/IsaacSpinner"; @@ -30,10 +30,15 @@ export const Concepts = withRouter((props: RouteComponentProps) => { maths: TAG_ID.maths, }; - const applicableTags = tags.getDirectDescendents(subjectToTagMap[subject ?? "physics"]); + const applicableTags = subject ? tags.getDirectDescendents(subjectToTagMap[subject]) : tags.allFieldTags; const tagCounts : Record = applicableTags.reduce((acc, t) => ({...acc, [t.id]: concepts?.filter(c => c.tags?.includes(t.id)).length || 0}), {}); - useEffect(() => {dispatch(fetchConcepts());}, [dispatch]); + useEffect(() => { + // TODO: run if EITHER subject exists OR there is no subject, but *not* if the subject is loading + // if (pageContext) { + dispatch(fetchConcepts(undefined, subject)); + // } + }, [dispatch, subject]); const searchParsed = queryString.parse(location.search); @@ -44,9 +49,10 @@ export const Concepts = withRouter((props: RouteComponentProps) => { const filters = (Array.isArray(filterParsed) ? filterParsed[0] || "" : filterParsed || "").split(","); const [searchText, setSearchText] = useState(query); - const [conceptFilters, setConceptFilters] = useState( - applicableTags.filter(f => filters.includes(f.id)) - ); + const [conceptFilters, setConceptFilters] = useState([ + ...(subject ? [tags.getById(subjectToTagMap[subject])] : []), + ...applicableTags.filter(f => filters.includes(f.id)) + ]); const [shortcutResponse, setShortcutResponse] = useState(); @@ -54,7 +60,7 @@ export const Concepts = withRouter((props: RouteComponentProps) => { if (e) { e.preventDefault(); } - pushConceptsToHistory(history, searchText || "", conceptFilters.map(f => f.id)); + pushConceptsToHistory(history, searchText || "", [...conceptFilters.map(f => f.id)]); if (searchText) { setShortcutResponse(shortcuts(searchText)); @@ -87,6 +93,8 @@ export const Concepts = withRouter((props: RouteComponentProps) => { const crumb = isPhy && isDefinedContext(pageContext) && generateSubjectLandingPageCrumbFromContext(pageContext); + const sidebarProps = {searchText, setSearchText, conceptFilters, setConceptFilters, applicableTags, tagCounts}; + return ( { icon={{type: "hex", icon: "page-icon-concept"}} /> - {pageContext?.subject ? : } + {pageContext?.subject + ? + : + } - + {isPhy &&
    + {shortcutAndFilteredSearchResults &&
    + Showing {shortcutAndFilteredSearchResults.length} results +
    } + + {shortcutAndFilteredSearchResults + ? + : No results found + } +
    + } + + {isAda &&

    - Search Results {query != "" ? shortcutAndFilteredSearchResults ? {shortcutAndFilteredSearchResults.length} : : null} + Search Results + {query !== "" + ? shortcutAndFilteredSearchResults + ? {shortcutAndFilteredSearchResults.length} + : + : null + }

    - + {shortcutAndFilteredSearchResults ? - isPhy ? - : - + : No results found} -     +     }     diff --git a/src/scss/phy/checkbox.scss b/src/scss/phy/checkbox.scss index 78e643ed3a..70e95eda23 100644 --- a/src/scss/phy/checkbox.scss +++ b/src/scss/phy/checkbox.scss @@ -17,6 +17,10 @@ .styled-checkbox-wrapper div input[type="checkbox"] { border: none; + + &[color="theme"] { + --checkbox-selected-color: var(--subject-color-500); + } } .icon-checkbox-off { @@ -73,7 +77,7 @@ } &::after { - background-color: $color-brand-500; + background-color: var(--checkbox-selected-color, $color-brand-500); } } @@ -85,7 +89,7 @@ ); &::before { - background-color: $color-brand-500; + background-color: var(--checkbox-selected-color, $color-brand-500); } &::after { background-color: $color-neutral-100; diff --git a/src/scss/phy/list-groups.scss b/src/scss/phy/list-groups.scss index ad0a372471..bfee7063d0 100644 --- a/src/scss/phy/list-groups.scss +++ b/src/scss/phy/list-groups.scss @@ -92,3 +92,7 @@ margin: 0.5rem 0.5rem 0 0; font-size: 14px; } + +.list-results-container { + background-color: $color-neutral-100; +} From e1de4c05d3d2f0d26974774cb6f6d7075d06dbe1 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Mon, 10 Mar 2025 10:29:43 +0000 Subject: [PATCH 4/9] Clear descendent tags when re-selecting a subject tag --- src/app/components/elements/layout/SidebarLayout.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/components/elements/layout/SidebarLayout.tsx b/src/app/components/elements/layout/SidebarLayout.tsx index 4e4c9cfc79..84d18ede94 100644 --- a/src/app/components/elements/layout/SidebarLayout.tsx +++ b/src/app/components/elements/layout/SidebarLayout.tsx @@ -329,7 +329,7 @@ export const GenericConceptsSidebar = (props: ConceptListSidebarProps) => { return
    {isSelected &&
    From 7583c4065a5ecff83df76167419ffc779ba3850d Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Mon, 10 Mar 2025 10:33:17 +0000 Subject: [PATCH 5/9] Overhaul state management for previous page context --- src/IsaacAppTypes.tsx | 1 + src/app/components/pages/Concept.tsx | 14 ++---- src/app/components/pages/Concepts.tsx | 17 +++---- src/app/components/pages/Question.tsx | 14 ++---- src/app/components/pages/QuestionFinder.tsx | 2 +- src/app/services/pageContext.ts | 53 ++++++++++++++------- src/app/state/selectors.tsx | 1 + src/app/state/slices/context.ts | 12 +++-- 8 files changed, 61 insertions(+), 53 deletions(-) diff --git a/src/IsaacAppTypes.tsx b/src/IsaacAppTypes.tsx index 421ebd3e9c..8b2e46c9a1 100644 --- a/src/IsaacAppTypes.tsx +++ b/src/IsaacAppTypes.tsx @@ -755,4 +755,5 @@ export type QuestionCorrectness = "CORRECT" | "INCORRECT" | "NOT_ANSWERED" | "NO export type PageContextState = { stage?: LearningStage[]; subject?: Subject; + previousContext?: Omit; } | null | undefined; diff --git a/src/app/components/pages/Concept.tsx b/src/app/components/pages/Concept.tsx index e8b9cf660c..c1c94a3e86 100644 --- a/src/app/components/pages/Concept.tsx +++ b/src/app/components/pages/Concept.tsx @@ -1,11 +1,11 @@ import React, {useEffect} from "react"; import {withRouter} from "react-router-dom"; -import {AppState, fetchDoc, pageContextSlice, selectors, useAppDispatch, useAppSelector} from "../../state"; +import {AppState, fetchDoc, selectors, useAppDispatch, useAppSelector} from "../../state"; import {Col, Container, Row} from "reactstrap"; import {ShowLoading} from "../handlers/ShowLoading"; import {IsaacContent} from "../content/IsaacContent"; import {IsaacConceptPageDTO} from "../../../IsaacApiTypes"; -import {DOCUMENT_TYPE, Subject, above, below, getUpdatedPageContext, isAda, isPhy, useDeviceSize, useNavigation} from "../../services"; +import {DOCUMENT_TYPE, Subject, above, below, usePreviousPageContext, isAda, isPhy, useDeviceSize, useNavigation} from "../../services"; import {DocumentSubject, GameboardContext} from "../../../IsaacAppTypes"; import {RelatedContent} from "../elements/RelatedContent"; import {WithFigureNumbering} from "../elements/WithFigureNumbering"; @@ -35,18 +35,12 @@ export const Concept = withRouter(({match: {params}, location: {search}, concept const dispatch = useAppDispatch(); const conceptId = conceptIdOverride || params.conceptId; const user = useAppSelector(selectors.user.orNull); - const prevPageContext = useAppSelector(selectors.pageContext.context); useEffect(() => {dispatch(fetchDoc(DOCUMENT_TYPE.CONCEPT, conceptId));}, [conceptId]); const doc = useAppSelector((state: AppState) => state?.doc || null); const navigation = useNavigation(doc); const deviceSize = useDeviceSize(); - useEffect(() => { - if (doc && doc !== 404) { - const newPageContext = getUpdatedPageContext(prevPageContext, user && user.loggedIn && user.registeredContexts || undefined, doc); - dispatch(pageContextSlice.actions.updatePageContext(newPageContext)); - } - }, [dispatch, user, doc]); + const pageContext = usePreviousPageContext(user && user.loggedIn && user.registeredContexts || undefined, doc && doc !== 404 ? doc : undefined); const ManageButtons = () =>
    @@ -63,7 +57,7 @@ export const Concept = withRouter(({match: {params}, location: {search}, concept return { const doc = supertypedDoc as IsaacConceptPageDTO & DocumentSubject; return - + { const dispatch = useAppDispatch(); const user = useAppSelector(selectors.user.orNull); const concepts = useAppSelector((state: AppState) => state?.concepts?.results || null); - const pageContext = useUrlPageTheme({resetIfNotFound: true}); - - const subject = useAppSelector(selectors.pageContext.subject); + const pageContext = useUrlPageTheme(); const subjectToTagMap = { physics: TAG_ID.physics, @@ -30,15 +28,14 @@ export const Concepts = withRouter((props: RouteComponentProps) => { maths: TAG_ID.maths, }; - const applicableTags = subject ? tags.getDirectDescendents(subjectToTagMap[subject]) : tags.allFieldTags; + const applicableTags = pageContext?.subject ? tags.getDirectDescendents(subjectToTagMap[pageContext.subject]) : tags.allFieldTags; const tagCounts : Record = applicableTags.reduce((acc, t) => ({...acc, [t.id]: concepts?.filter(c => c.tags?.includes(t.id)).length || 0}), {}); useEffect(() => { - // TODO: run if EITHER subject exists OR there is no subject, but *not* if the subject is loading - // if (pageContext) { - dispatch(fetchConcepts(undefined, subject)); - // } - }, [dispatch, subject]); + if (pageContext) { + dispatch(fetchConcepts(undefined, pageContext?.subject)); + } + }, [dispatch]); const searchParsed = queryString.parse(location.search); @@ -50,7 +47,7 @@ export const Concepts = withRouter((props: RouteComponentProps) => { const [searchText, setSearchText] = useState(query); const [conceptFilters, setConceptFilters] = useState([ - ...(subject ? [tags.getById(subjectToTagMap[subject])] : []), + ...(pageContext?.subject ? [tags.getById(subjectToTagMap[pageContext.subject])] : []), ...applicableTags.filter(f => filters.includes(f.id)) ]); diff --git a/src/app/components/pages/Question.tsx b/src/app/components/pages/Question.tsx index 0c8a4a1c64..5bb66cefb3 100644 --- a/src/app/components/pages/Question.tsx +++ b/src/app/components/pages/Question.tsx @@ -1,7 +1,7 @@ import React, {useEffect} from "react"; import {Button, Col, Container, Row} from "reactstrap"; import {match, RouteComponentProps, withRouter} from "react-router-dom"; -import {fetchDoc, goToSupersededByQuestion, pageContextSlice, selectors, useAppDispatch, useAppSelector} from "../../state"; +import {fetchDoc, goToSupersededByQuestion, selectors, useAppDispatch, useAppSelector} from "../../state"; import {ShowLoading} from "../handlers/ShowLoading"; import {IsaacQuestionPageDTO} from "../../../IsaacApiTypes"; import { @@ -9,7 +9,7 @@ import { DOCUMENT_TYPE, fastTrackProgressEnabledBoards, generateQuestionTitle, - getUpdatedPageContext, + usePreviousPageContext, isAda, isPhy, isStudent, @@ -63,7 +63,6 @@ export const Question = withRouter(({questionIdOverride, match, location, previe const questionId = questionIdOverride || match.params.questionId; const doc = useAppSelector(selectors.doc.get); const user = useAppSelector(selectors.user.orNull); - const prevPageContext = useAppSelector(selectors.pageContext.context); const navigation = useNavigation(doc); const pageContainsLLMFreeTextQuestion = useAppSelector(selectors.questions.includesLLMFreeTextQuestion); const query = queryString.parse(location.search); @@ -74,12 +73,7 @@ export const Question = withRouter(({questionIdOverride, match, location, previe dispatch(fetchDoc(DOCUMENT_TYPE.QUESTION, questionId)); }, [dispatch, questionId]); - useEffect(() => { - if (doc && doc !== 404) { - const newPageContext = getUpdatedPageContext(prevPageContext, user && user.loggedIn && user.registeredContexts || undefined, doc); - dispatch(pageContextSlice.actions.updatePageContext(newPageContext)); - } - }, [dispatch, user, doc]); + const pageContext = usePreviousPageContext(user && user.loggedIn && user.registeredContexts || undefined, doc && doc !== 404 ? doc : undefined); return { const doc = supertypedDoc as IsaacQuestionPageDTO & DocumentSubject; @@ -87,7 +81,7 @@ export const Question = withRouter(({questionIdOverride, match, location, previe const isFastTrack = doc && doc.type === DOCUMENT_TYPE.FAST_TRACK_QUESTION; return - + { const user = useAppSelector((state: AppState) => state && state.user); const params = useQueryParams(false); const history = useHistory(); - const pageContext = useUrlPageTheme({resetIfNotFound: true}); + const pageContext = useUrlPageTheme(); const isSolitaryStage = pageStageToSearchStage(pageContext?.stage).length === 1; const [searchTopics, setSearchTopics] = useState(arrayFromPossibleCsv(params.topics)); diff --git a/src/app/services/pageContext.ts b/src/app/services/pageContext.ts index 33cd71b2b0..d46915ce71 100644 --- a/src/app/services/pageContext.ts +++ b/src/app/services/pageContext.ts @@ -4,8 +4,8 @@ import { LearningStage, LearningStages, PHY_NAV_SUBJECTS, SiteTheme, STAGE_TO_LE import { isDefined } from "./miscUtils"; import { useLocation } from "react-router"; import { HUMAN_STAGES, HUMAN_SUBJECTS } from "./constants"; -import { pageContextSlice, useAppDispatch } from "../state"; -import { useEffect } from "react"; +import { pageContextSlice, selectors, useAppDispatch, useAppSelector } from "../state"; +import { useEffect, useMemo } from "react"; const filterBySubjects = (tags: (TAG_ID | string)[]): SiteTheme[] => { // filtering this const list against the passed-in tags maintains the order (and thus precedence) of the subjects @@ -64,8 +64,11 @@ export const getThemeFromTags = (tags?: (TAG_ID | string)[]): SiteTheme => { * @param doc - The current page DTO. The audience and tags of this object will be used to determine the new context. * @returns The page context for this page. */ -export const getUpdatedPageContext = (previousContext: PageContextState | undefined, userContexts: readonly UserContext[] | undefined, doc: ContentBaseDTO | undefined): PageContextState => { - const newContext = {stage: undefined, subject: undefined} as NonNullable; +export const usePreviousPageContext = (userContexts: readonly UserContext[] | undefined, doc: ContentBaseDTO | undefined): PageContextState => { + const previousContext = useAppSelector(selectors.pageContext.previousContext) as PageContextState; + const dispatch = useAppDispatch(); + + const newContext = useMemo(() => ({stage: undefined, subject: undefined, previousContext} as NonNullable), [previousContext]); // if we haven't changed learning stage (GCSE => GCSE), use the learning stage from the old context if (previousContext?.stage && doc?.audience?.some(a => a.stage?.map(s => STAGE_TO_LEARNING_STAGE[s]).filter(isDefined).some(s => previousContext.stage?.includes(s)))) { @@ -104,6 +107,18 @@ export const getUpdatedPageContext = (previousContext: PageContextState | undefi } // otherwise we cannot infer a subject to show, so the default of "neutral" is used + useEffect(() => { + dispatch(pageContextSlice.actions.updatePageContext(newContext)); + + return () => { + dispatch(pageContextSlice.actions.updatePageContext({ + subject: undefined, + stage: undefined, + previousContext: {subject: newContext.subject, stage: newContext.stage}, + })); + }; + }, [dispatch, doc]); + return newContext; }; @@ -126,35 +141,37 @@ function isValidIsaacStage(stage?: string): stage is LearningStage { function determinePageContextFromUrl(url: string): PageContextState { const [subject, stage] = url.split("/").filter(Boolean); - if (isValidIsaacSubject(subject) && isValidIsaacStage(stage)) { - return {subject, stage: [stage]}; - } - return {}; + + return { + subject: isValidIsaacSubject(subject) ? subject : undefined, + stage: isValidIsaacStage(stage) ? [stage] : [], + } as PageContextState; } /** * A hook for updating the page context based on the URL. Only use on pages where the URL is the source of truth for the page context. * (i.e. subject-specific pages, like question finders, concept pages, etc.) * If you want to get the current page context from redux rather than the URL, use `useAppSelector(selectors.pageContext.context)` instead. - * @param resetIfNotFound - If true, the page context will be reset if the URL does not contain a valid page context. This should be true on pages with a "neutral" version and a "subject-specific" version. * @returns The current page context. */ -export function useUrlPageTheme(params?: {resetIfNotFound?: boolean}): PageContextState { +export function useUrlPageTheme(): PageContextState { const location = useLocation(); const dispatch = useAppDispatch(); useEffect(() => { const urlContext = determinePageContextFromUrl(location.pathname); - if (urlContext?.subject && urlContext?.stage) { + dispatch(pageContextSlice.actions.updatePageContext({ + subject: urlContext?.subject, + stage: urlContext?.stage, + })); + + return () => { dispatch(pageContextSlice.actions.updatePageContext({ - subject: urlContext.subject, - stage: urlContext.stage + subject: undefined, + stage: undefined, + previousContext: {subject: urlContext?.subject, stage: urlContext?.stage}, })); - } else { - if (params?.resetIfNotFound) { - dispatch(pageContextSlice.actions.resetPageContext()); - } - } + }; }, [dispatch, location.pathname]); return determinePageContextFromUrl(location.pathname); diff --git a/src/app/state/selectors.tsx b/src/app/state/selectors.tsx index 0fd0a7e645..e36f07a96b 100644 --- a/src/app/state/selectors.tsx +++ b/src/app/state/selectors.tsx @@ -74,6 +74,7 @@ export const selectors = { pageContext: { context: (state: AppState) => state?.pageContext ?? undefined, // transform null => undefined + previousContext: (state: AppState) => state?.pageContext?.previousContext ?? undefined, stage: (state: AppState) => state?.pageContext?.stage, subject: (state: AppState) => state?.pageContext?.subject, } diff --git a/src/app/state/slices/context.ts b/src/app/state/slices/context.ts index 45c9651c8f..1719673056 100644 --- a/src/app/state/slices/context.ts +++ b/src/app/state/slices/context.ts @@ -1,6 +1,5 @@ import { createSlice } from "@reduxjs/toolkit"; import { PageContextState } from "../../../IsaacAppTypes"; -import { Stage } from "../../../IsaacApiTypes"; interface actionType { payload: PageContextState; @@ -13,9 +12,14 @@ export const pageContextSlice = createSlice({ reducers: { updatePageContext: (state, action: actionType) => ({ ...state, - stage: action.payload?.stage, - subject: action.payload?.subject, + stage: action.payload?.stage ?? state?.stage, + subject: action.payload?.subject ?? state?.subject, + previousContext: action.payload?.previousContext ?? state?.previousContext, + }), + resetPageContext: (state) => ({ + ...state, + stage: undefined, + subject: undefined, }), - resetPageContext: () => null, }, }); From eb7d5d26bf044842ab921fa33382aae26644d171 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Mon, 10 Mar 2025 11:03:48 +0000 Subject: [PATCH 6/9] Prevent accidental inheritance of previous context on undefined context --- src/app/services/pageContext.ts | 1 + src/app/state/slices/context.ts | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/app/services/pageContext.ts b/src/app/services/pageContext.ts index d46915ce71..5b3c5862a3 100644 --- a/src/app/services/pageContext.ts +++ b/src/app/services/pageContext.ts @@ -163,6 +163,7 @@ export function useUrlPageTheme(): PageContextState { dispatch(pageContextSlice.actions.updatePageContext({ subject: urlContext?.subject, stage: urlContext?.stage, + previousContext: {subject: urlContext?.subject, stage: urlContext?.stage}, })); return () => { diff --git a/src/app/state/slices/context.ts b/src/app/state/slices/context.ts index 1719673056..a5c44f25bc 100644 --- a/src/app/state/slices/context.ts +++ b/src/app/state/slices/context.ts @@ -12,8 +12,9 @@ export const pageContextSlice = createSlice({ reducers: { updatePageContext: (state, action: actionType) => ({ ...state, - stage: action.payload?.stage ?? state?.stage, - subject: action.payload?.subject ?? state?.subject, + // stage and subject can be undefined, so should not ??-inherit from the previous context + stage: action.payload?.stage, + subject: action.payload?.subject, previousContext: action.payload?.previousContext ?? state?.previousContext, }), resetPageContext: (state) => ({ From 4f2f2a6372bb1fcc0f1131bf382e50c02e432073 Mon Sep 17 00:00:00 2001 From: Jacob Brown <47645360+jacbn@users.noreply.github.com> Date: Mon, 10 Mar 2025 11:19:23 +0000 Subject: [PATCH 7/9] Fix 614: Unused variable, import, function or class Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- src/app/components/elements/layout/SidebarLayout.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/components/elements/layout/SidebarLayout.tsx b/src/app/components/elements/layout/SidebarLayout.tsx index 84d18ede94..2fc918aab6 100644 --- a/src/app/components/elements/layout/SidebarLayout.tsx +++ b/src/app/components/elements/layout/SidebarLayout.tsx @@ -3,7 +3,7 @@ import { Col, ColProps, RowProps, Input, Offcanvas, OffcanvasBody, OffcanvasHead import partition from "lodash/partition"; import classNames from "classnames"; import { AssignmentDTO, ContentSummaryDTO, IsaacConceptPageDTO, QuestionDTO, QuizAttemptDTO, RegisteredUserDTO } from "../../../../IsaacApiTypes"; -import { above, ACCOUNT_TAB, ACCOUNT_TABS, AUDIENCE_DISPLAY_FIELDS, below, BOARD_ORDER_NAMES, BoardCompletions, BoardCreators, BoardLimit, BoardSubjects, BoardViews, confirmThen, determineAudienceViews, filterAssignmentsByStatus, filterAudienceViewsByProperties, getDistinctAssignmentGroups, getDistinctAssignmentSetters, getHumanContext, getThemeFromContextAndTags, HUMAN_STAGES, HUMAN_SUBJECTS, ifKeyIsEnter, isAda, isDefined, PHY_NAV_SUBJECTS, siteSpecific, TAG_ID, tags, useDeviceSize } from "../../../services"; +import { above, ACCOUNT_TAB, ACCOUNT_TABS, AUDIENCE_DISPLAY_FIELDS, below, BOARD_ORDER_NAMES, BoardCompletions, BoardCreators, BoardLimit, BoardSubjects, BoardViews, confirmThen, determineAudienceViews, filterAssignmentsByStatus, filterAudienceViewsByProperties, getDistinctAssignmentGroups, getDistinctAssignmentSetters, getHumanContext, getThemeFromContextAndTags, HUMAN_STAGES, ifKeyIsEnter, isAda, isDefined, PHY_NAV_SUBJECTS, siteSpecific, TAG_ID, tags, useDeviceSize } from "../../../services"; import { StageAndDifficultySummaryIcons } from "../StageAndDifficultySummaryIcons"; import { selectors, useAppSelector } from "../../../state"; import { Link, useHistory } from "react-router-dom"; From 0c2c7056a71375480c3a6109547d8ffff3afd477 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Mon, 10 Mar 2025 17:35:35 +0000 Subject: [PATCH 8/9] Fix QF truthiness bug caused by empty array in pageContext's `stage` --- .../components/elements/panels/QuestionFinderFilterPanel.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx b/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx index 8622fdc5d2..f4fa5d39ef 100644 --- a/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx +++ b/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx @@ -176,6 +176,8 @@ export function QuestionFinderFilterPanel(props: QuestionFinderFilterPanelProps) } }; + console.log(getFilteredStageOptions()); + return
    { // the filters panel can only be collapsed when it is not a sidebar @@ -220,7 +222,7 @@ export function QuestionFinderFilterPanel(props: QuestionFinderFilterPanelProps) toggle={() => listStateDispatch({type: "toggle", id: "stage", focus: below["md"](deviceSize)})} numberSelected={(isAda && searchStages.includes(STAGE.ALL)) ? searchStages.length - 1 : searchStages.length} > - {getFilteredStageOptions().filter(stage => pageStageToSearchStage(pageContext?.stage).includes(stage.value) || !pageContext?.stage).map((stage, index) => ( + {getFilteredStageOptions().filter(stage => pageStageToSearchStage(pageContext?.stage).includes(stage.value) || !pageContext?.stage?.length).map((stage, index) => (
    Date: Wed, 12 Mar 2025 13:31:44 +0000 Subject: [PATCH 9/9] Remove console.log --- .../components/elements/panels/QuestionFinderFilterPanel.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx b/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx index f4fa5d39ef..434ba212b3 100644 --- a/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx +++ b/src/app/components/elements/panels/QuestionFinderFilterPanel.tsx @@ -176,8 +176,6 @@ export function QuestionFinderFilterPanel(props: QuestionFinderFilterPanelProps) } }; - console.log(getFilteredStageOptions()); - return
    { // the filters panel can only be collapsed when it is not a sidebar