Skip to content

Sidebar container refactor #1995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jacbn wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Sidebar container refactor #1995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9be713d
Rework most pages' sidebar container
jacbn Feb 17, 2026
9ccf8c3
Restore Sci sidebar layout; import, docs
jacbn Feb 18, 2026
fd921a8
Remove unused import (eslint)
jacbn Feb 18, 2026
bec70fd
More ESLint fixes
jacbn Feb 18, 2026
4253087
Align all `sidebar=` props for easier regex checking
jacbn Feb 18, 2026
aa3ac3d
Correctly use `pageTitle` prop in QF
jacbn Feb 18, 2026
0287ab9
Update VRT baselines
actions-user Feb 18, 2026
1afd557
Merge pull request #1996 from isaacphysics/vrt/improvement/sidebar-co…
jacbn Feb 18, 2026
f83d415
Restore old Account page width for Ada
jacbn Feb 25, 2026
723fb63
Adjust Sci sidebar padding
jacbn Feb 25, 2026
a59557c
Significantly reduce margin below pages to do separately
jacbn Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/IsaacAppTypes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ export const AssignmentScheduleContext = React.createContext<{
setCollapsed: (b: boolean) => void;
viewBy: "startDate" | "dueDate";
}>({boardsById: {}, groupsById: {}, groupFilter: {}, boardIdsByGroupId: {}, groups: [], gameboards: [], openAssignmentModal: () => {}, collapsed: false, setCollapsed: () => {}, viewBy: "startDate"});
export const SidebarContext = React.createContext<{sidebarPresent: boolean} | undefined>(undefined);
export const ContentSidebarContext = React.createContext<{ toggle: () => void; close: () => void; } | undefined>(undefined);

export interface AuthorisedAssignmentProgress extends ApiTypes.AssignmentProgressDTO {
Expand Down
112 changes: 55 additions & 57 deletions src/app/components/elements/Book.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import React, {useEffect, useState} from "react";
import {Container} from "reactstrap";
import {MainContent, SidebarLayout} from "./layout/SidebarLayout";
import {Markup} from "./markup";
import {TitleAndBreadcrumb} from "./TitleAndBreadcrumb";
import {BOOK_DETAIL_ID_SEPARATOR, BOOKS_CRUMB, useContextFromContentObjectTags} from "../../services";
import {BOOK_DETAIL_ID_SEPARATOR, BOOKS_CRUMB, siteSpecific, useContextFromContentObjectTags} from "../../services";
import {useLocation, useParams} from "react-router";
import {useGetBookDetailPageQuery, useGetBookIndexPageQuery} from "../../state/slices/api/booksApi";
import {BookPage} from "./BookPage";
Expand All @@ -14,6 +12,7 @@ import {ContentDTO} from "../../../IsaacApiTypes";
import { PageMetadata } from "./PageMetadata";
import { WithFigureNumbering } from "./WithFigureNumbering";
import { ContentControlledSidebar } from "./sidebar/ContentControlledSidebar";
import { PageContainer } from "./layout/PageContainer";

export const Book = () => {

Expand Down Expand Up @@ -41,59 +40,58 @@ export const Book = () => {
setPageId(fragmentId);
}, [book?.id, location.pathname]);

return <Container data-bs-theme={pageContext?.subject ?? "neutral"}>
<TitleAndBreadcrumb
currentPageTitle={pageId === undefined ? "Book" : book?.title ?? "Book"}
icon={{type: "icon", icon: "icon-book"}}
intermediateCrumbs={pageId !== undefined && book?.title ? [BOOKS_CRUMB, {title: book.title, to: `/books/${bookId}`}] : [BOOKS_CRUMB]}
/>
<SidebarLayout>
<ShowLoadingQuery
query={bookIndexPageQuery}
defaultErrorTitle="Unable to load book contents"
thenRender={(definedBookIndexPage) => {
return <>
<ContentControlledSidebar sidebar={book?.sidebar} hideButton/>
<MainContent>
{pageId
? <ShowLoadingQuery
query={bookDetailPageQuery}
defaultErrorTitle="Unable to load book page"
thenRender={(bookDetailPage) => {
return <WithFigureNumbering doc={bookDetailPage}>
<BookPage page={bookDetailPage} />
</WithFigureNumbering>;
}}
/>
: <>
<PageMetadata doc={definedBookIndexPage} showSidebarButton sidebarButtonText={book?.sidebar?.subtitle}/>
{definedBookIndexPage.value && <div>
<div className="book-image-container book-height-lg d-none d-sm-block mx-3 float-end">
<img src={definedBookIndexPage.coverImage?.src} alt={definedBookIndexPage.title} />
</div>
<Markup className="d-contents" trusted-markup-encoding={"markdown"}>{definedBookIndexPage.value}</Markup>
</div>}
{!!definedBookIndexPage.children?.length && <>
<div className="d-flex">
<div className="flex-grow-1">
<WithFigureNumbering doc={definedBookIndexPage}>
<IsaacContentValueOrChildren {...definedBookIndexPage.children[0] as ContentDTO} />
</WithFigureNumbering>
</div>
<div className="book-image-container book-height-lg d-none d-sm-block mx-3 float-end">
<img src={definedBookIndexPage.coverImage?.src} alt={definedBookIndexPage.title} />
</div>
</div>
<IsaacContentValueOrChildren>
{definedBookIndexPage.children.slice(1)}
</IsaacContentValueOrChildren>
</>}
</>
}
</MainContent>
</>;
}}
return <PageContainer data-bs-theme={pageContext?.subject ?? "neutral"}
pageTitle={
<TitleAndBreadcrumb
currentPageTitle={pageId === undefined ? "Book" : book?.title ?? "Book"}
icon={{type: "icon", icon: "icon-book"}}
intermediateCrumbs={pageId !== undefined && book?.title ? [BOOKS_CRUMB, {title: book.title, to: `/books/${bookId}`}] : [BOOKS_CRUMB]}
/>
</SidebarLayout>
</Container>;
}
sidebar={siteSpecific(
<ContentControlledSidebar sidebar={book?.sidebar} />,
undefined
)}
>
<ShowLoadingQuery
query={bookIndexPageQuery}
defaultErrorTitle="Unable to load book contents"
thenRender={(definedBookIndexPage) => {
return pageId
? <ShowLoadingQuery
query={bookDetailPageQuery}
defaultErrorTitle="Unable to load book page"
thenRender={(bookDetailPage) => {
return <WithFigureNumbering doc={bookDetailPage}>
<BookPage page={bookDetailPage} />
</WithFigureNumbering>;
}}
/>
: <>
<PageMetadata doc={definedBookIndexPage} showSidebarButton sidebarButtonText={book?.sidebar?.subtitle}/>
{definedBookIndexPage.value && <div>
<div className="book-image-container book-height-lg d-none d-sm-block mx-3 float-end">
<img src={definedBookIndexPage.coverImage?.src} alt={definedBookIndexPage.title} />
</div>
<Markup className="d-contents" trusted-markup-encoding={"markdown"}>{definedBookIndexPage.value}</Markup>
</div>}
{!!definedBookIndexPage.children?.length && <>
<div className="d-flex">
<div className="flex-grow-1">
<WithFigureNumbering doc={definedBookIndexPage}>
<IsaacContentValueOrChildren {...definedBookIndexPage.children[0] as ContentDTO} />
</WithFigureNumbering>
</div>
<div className="book-image-container book-height-lg d-none d-sm-block mx-3 float-end">
<img src={definedBookIndexPage.coverImage?.src} alt={definedBookIndexPage.title} />
</div>
</div>
<IsaacContentValueOrChildren>
{definedBookIndexPage.children.slice(1)}
</IsaacContentValueOrChildren>
</>}
</>;
}}
/>
</PageContainer>;
};
56 changes: 56 additions & 0 deletions src/app/components/elements/layout/PageContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React from "react";
import { Container, ContainerProps } from "reactstrap";
import { siteSpecific } from "../../../services";
import { MainContent, SidebarLayout } from "./SidebarLayout";
import classNames from "classnames";

interface PageContainerProps extends Omit<ContainerProps, "pageTitle"> {
pageTitle?: React.ReactNode;
sidebar?: React.ReactNode;
}

/**
* A component to manage the main layout of a page, including the page title and sidebar if required.
* @param props The props for the PageContainer component.
* @param props.pageTitle The title of the page, displayed above both the main content and the sidebar.
* @param props.sidebar The content of the sidebar. Displayed according to @see SidebarLayout. If not provided, the page will be displayed without a sidebar.
* @param props.children The main content of the page.
* @returns A React component that renders the page layout.
*/
export const PageContainer = (props: PageContainerProps) => {
const { children, sidebar, pageTitle, id, ...rest } = props;
// TODO increase mb-2 to ~mb-7, but carefully consider mobile layouts and remove inconsistent additional spacing below individual pages.
if (!sidebar) {
return <Container {...rest} id={id} className={classNames("mb-2", rest.className)}>
{pageTitle}
{children}
</Container>;
}

return siteSpecific(
// Sci
<Container {...rest} id={id} className={classNames("mb-2", rest.className)}>
{pageTitle}
<SidebarLayout show={!!sidebar}>
{sidebar}
<MainContent>
{children}
</MainContent>
</SidebarLayout>
</Container>,

// Ada
// The ID is applied to the top-level component here to ensure #id:before / :after background elements cover the entire page.
// Slightly annoying since the className feels like it should be on the Container, leaving this awkward split.
// Maybe revisit this when we have more use cases?
<SidebarLayout className="g-md-0" id={id} show={!!sidebar}>
{sidebar}
<MainContent className="overflow-x-auto">
<Container fluid {...rest} className={classNames("my-ada-container mw-1600 px-md-4 px-lg-6 mb-2", rest.className)}>
{pageTitle}
{children}
</Container>
</MainContent>
</SidebarLayout>
);
};
32 changes: 21 additions & 11 deletions src/app/components/elements/layout/SidebarLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
import React, { useEffect } from "react";
import { Col, ColProps, RowProps, Offcanvas, OffcanvasBody, OffcanvasHeader, Row } from "reactstrap";
import React, { useContext, useEffect } from "react";
import { Col, ColProps, RowProps, Offcanvas, OffcanvasBody, OffcanvasHeader } from "reactstrap";
import classNames from "classnames";
import { above, isAda, siteSpecific, useDeviceSize } from "../../../services";
import { above, isPhy, siteSpecific, useDeviceSize } from "../../../services";
import { mainContentIdSlice, selectors, sidebarSlice, useAppDispatch, useAppSelector } from "../../../state";
import { ContentSidebarContext } from "../../../../IsaacAppTypes";
import { ContentSidebarContext, SidebarContext } from "../../../../IsaacAppTypes";
import { AffixButton } from "../AffixButton";
import { SidebarButton } from "../SidebarButton";

export const SidebarLayout = (props: RowProps) => {
const { className, ...rest } = props;
return siteSpecific(<Row {...rest} className={classNames("sidebar-layout", className)}/>, props.children);
interface SidebarLayoutProps extends RowProps {
show?: boolean;
}

export const SidebarLayout = (props: SidebarLayoutProps) => {
const { className, show=true, ...rest } = props;
return show
? <SidebarContext.Provider value={{sidebarPresent: true}}>
<div {...rest} className={classNames("d-flex flex-column sidebar-layout", siteSpecific("flex-lg-row", "flex-md-row"), className)}/>
Copy link
Contributor

@sjd210 sjd210 Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This className (rather than the simpler "sidebar-layout row" of before) is shifting everything on Sci horizontally such that sidebar and main content and now much closer together, the sidebar is smaller, and it doesn't line up as well with the left side of the page (i.e. aligning with the title hex icon/my isaac/logo). I imagine it isn't intentional, but regardless I don't like it.

I'm also not entirely clear on what the class is meant to achieve. It seems to me from quickly looking at the My Ada branch that the "d-flex flex-column flex-md-row" is only needed for Ada and not Sci, since we use things like <Col ... xs={12} lg={8} on L32 to handle this kind of stacking behaviour on Sci. I think that this dichotomy is for the sake of sidebars being a fixed size on Ada, but a proportion of screen-size on Sci, but I'm not fully sure?

An easy solution is just to move the new classNames inside the Ada branch of siteSpecific, but it would be nicer for the content/sidebar stacking to work in a similar way across both sites, if at all possible.

Copy link
Contributor Author

@jacbn jacbn Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did this because we weren't really using row properly before. Even though it ended up looking fine, the row has negative margin (what with being a row and this being Bootstrap), but this does not match the parent .container's padding.

These two images are identical besides the box model, so you can see that the sidebar's padding overlaps the page container's padding badly:

image image

I suppose this would have meant that a new, fixed-width sidebar doesn't take up the width it claims to take up, which might have been why I changed it; I can't remember for certain though. Alternatively, this could have been solved by adding g-0 to give the same outcome while keeping the same sidebar-layout row structure, though all the changes below this point would still be required, and there might well have been reasons I am forgetting.

You are right, though, that the new approach ends up looking bad on Sci – but this is because the p-4 class that the sidebar has is now actually insetting on the left, rather than arbitrarily being shifted by -1.5rem:

image image

(aligned, at least!)

I much prefer this structure; we can then just kill off some of the left padding to restore it to its former glory (ps-1 most closely matches the title hexagon's spacing, but ps-3 most closely matches the old layout):
image

The only real difference left is that the row approach enforces spacing between the sidebar and the main content, which this new approach no longer does. I'd probably use gap in any other situation, but for Sci the sidebar:content ratio is fixed at either 1:2 or 1:3, and gap adds extra that causes overflow. I think my preferred alternative is just to increase the padding on the right of the sidebar, something like pe-5; this leaves the <MainContent> flush with the actual contents.

I'll make a commit with these changes on (appreciate long paragraphs are probably a bit hard to follow!) and would appreciate any thoughts from there.

Copy link
Contributor Author

@jacbn jacbn Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nicer for the content/sidebar stacking to work in a similar way across both sites, if at all possible.

As you mention before, this is quite difficult owing to the relative positioning of the sidebar and main content given the requirement for the Ada sidebar to exist flush with the left of the page. One possible improvement might be to replace <MainContent>'s <Col> lg / md sizings on L32 with just a div with flex-grow-1, which is how the Ada one should work, so that component is the same across both. But this is only possible because the sidebar itself has a fixed lg / md size on Sci, so the automatic sizing always gives (100% - sidebar%), which is precisely the sizings we would be replacing.

</SidebarContext.Provider>
: props.children;
};

export const MainContent = (props: ColProps) => {
Expand All @@ -29,10 +37,11 @@ export type SidebarProps = ColProps
export const NavigationSidebar = (props: SidebarProps) => {
// A navigation sidebar is used for external links that are supplementary to the main content (e.g. related content);
// the content in such a sidebar will collapse underneath the main content on smaller screens
if (isAda) return <></>;
const sidebarContext = useContext(SidebarContext);
if (!sidebarContext?.sidebarPresent) return <></>;

const { className, ...rest } = props;
return <Col tag="aside" aria-label="Sidebar" lg={4} xl={3} {...rest} className={classNames("sidebar no-print p-4 order-1 order-lg-0", className)} />;
return <Col tag="aside" aria-label="Sidebar" lg={4} xl={3} {...rest} className={classNames("sidebar no-print p-4 order-1 order-lg-0", {"ps-lg-3 py-lg-4 pe-lg-5": isPhy}, className)} />;
};

export interface ContentSidebarProps extends SidebarProps {
Expand All @@ -52,12 +61,13 @@ export const ContentSidebar = (props: ContentSidebarProps) => {

const pageTheme = useAppSelector(selectors.pageContext.subject);

if (isAda) return <></>;
const sidebarContext = useContext(SidebarContext);
if (!sidebarContext?.sidebarPresent) return <></>;

const { className, buttonTitle, hideButton, optionBar, ...rest } = props;
return <>
{above['lg'](deviceSize)
? <Col tag="aside" data-testid="sidebar" aria-label="Sidebar" lg={4} xl={3} {...rest} className={classNames("d-none d-lg-flex flex-column sidebar no-print p-4 order-0", className)} />
? <Col tag="aside" data-testid="sidebar" aria-label="Sidebar" lg={4} xl={3} {...rest} className={classNames("d-none d-lg-flex flex-column sidebar no-print p-4 order-0", {"ps-lg-3 py-lg-4 pe-lg-5": isPhy}, className)} />
: <>
{optionBar && <div className="d-flex align-items-center no-print flex-wrap py-3 gap-3">
<div className="flex-grow-1 d-inline-grid align-items-end">{optionBar}</div>
Expand Down
2 changes: 1 addition & 1 deletion src/app/components/elements/quiz/QuizContentsComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ export function QuizContentsComponent(props: QuizAttemptProps | QuizViewProps) {

return <>
<QuizTitle {...props} />
<SidebarLayout>
<SidebarLayout show={isPhy}>
<QuizSidebar {...sidebarProps} />
<MainContent>
{props.page === null || props.page == undefined ? QuizOverview({...{viewingAsSomeoneElse, ...props}}): <QuizQuestions {...props} page={props.page} /> }
Expand Down
5 changes: 3 additions & 2 deletions src/app/components/elements/quiz/QuizSidebarLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React, { ReactNode } from "react";
import { SidebarLayout, MainContent } from "../layout/SidebarLayout";
import { isPhy } from "../../../services";

export const QuizSidebarLayout = ({ children } : { children: ReactNode }) =>
<SidebarLayout className="d-flex flex-column align-items-end">
<SidebarLayout show={isPhy} className="d-flex flex-column align-items-end">
<MainContent>
<div className="d-flex border-top pt-2 my-2 align-items-center">
{children}
</div>
</MainContent>
</SidebarLayout>;
</SidebarLayout>;
2 changes: 1 addition & 1 deletion src/app/components/handlers/ShowLoadingQuery.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ type ShowLoadingQueryProps<T> = ShowLoadingQueryErrorProps<T> & ({
children?: undefined;
} | {
thenRender?: undefined;
children: JSX.Element | JSX.Element[];
children: React.ReactNode;
});
// A flexible way of displaying whether a RTKQ query is loading or errored. You can give as props:
// - Either: `children` or `thenRender` (a function that takes the query data and returns a React element)
Expand Down
34 changes: 18 additions & 16 deletions src/app/components/pages/AnvilAppsListing.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import React from "react";
import { Container } from "reactstrap";
import { generateSubjectLandingPageCrumbFromContext, TitleAndBreadcrumb } from "../elements/TitleAndBreadcrumb";
import { getHumanContext, isFullyDefinedContext, isSingleStageContext, useUrlPageTheme, VALID_APPS_CONTEXTS } from "../../services";
import { MainContent, SidebarLayout } from "../elements/layout/SidebarLayout";
import { getHumanContext, isFullyDefinedContext, isSingleStageContext, siteSpecific, useUrlPageTheme, VALID_APPS_CONTEXTS } from "../../services";
import { PageMetadata } from "../elements/PageMetadata";
import { PageFragment } from "../elements/PageFragment";
import { AnvilAppsListingSidebar } from "../elements/sidebar/AnvilAppsListingSidebar";
import { PageContainer } from "../elements/layout/PageContainer";

export const AnvilAppsListing = () => {
const pageContext = useUrlPageTheme();
Expand All @@ -26,18 +26,20 @@ export const AnvilAppsListing = () => {
</Container>;
}

return <Container data-bs-theme={pageContext?.subject}>
<TitleAndBreadcrumb
currentPageTitle={pageContext.stage[0] === "university" ? "Skills practice" : "Core skills practice"}
intermediateCrumbs={crumb ? [crumb] : []}
icon={{icon: "icon-revision", type: "icon"}}
/>
<SidebarLayout>
<AnvilAppsListingSidebar />
<MainContent>
<PageMetadata />
<PageFragment fragmentId={VALID_APPS_CONTEXTS[pageContext.subject]?.[pageContext.stage[0]] ?? ""} />
</MainContent>
</SidebarLayout>
</Container>;
return <PageContainer data-bs-theme={pageContext?.subject}
pageTitle={
<TitleAndBreadcrumb
currentPageTitle={pageContext.stage[0] === "university" ? "Skills practice" : "Core skills practice"}
intermediateCrumbs={crumb ? [crumb] : []}
icon={{icon: "icon-revision", type: "icon"}}
/>
}
sidebar={siteSpecific(
<AnvilAppsListingSidebar />,
undefined
)}
>
<PageMetadata />
<PageFragment fragmentId={VALID_APPS_CONTEXTS[pageContext.subject]?.[pageContext.stage[0]] ?? ""} />
</PageContainer>;
};
Loading