From ff5e446616dc759acbe8061cea66bc43b7406281 Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Mon, 21 Sep 2026 14:25:56 -0400 Subject: [PATCH 1/2] Add hover definition to Active learners, fix clipped month label --- .../Analytics/EngagementTrendChart.tsx | 47 ++++++++++++++++++- .../DashboardPage/Analytics/charts.test.tsx | 13 +++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx b/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx index 647df5d51d..1464b4b3a0 100644 --- a/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx +++ b/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx @@ -2,7 +2,8 @@ import React from "react" import { LineChart } from "@mui/x-charts/LineChart" -import { Skeleton, styled, useTheme } from "ol-components" +import { RiInformationLine } from "@remixicon/react" +import { Skeleton, styled, Tooltip, useTheme } from "ol-components" import type { MonthlyEngagementTrend } from "api/analytics-hooks/organizations" import { EmptyTableMessage, @@ -66,6 +67,15 @@ const TableWrapper = styled.div(({ theme }) => ({ borderTop: `1px solid ${theme.custom.colors.lightGray2}`, })) +const InfoTrigger = styled.span(({ theme }) => ({ + display: "inline-flex", + verticalAlign: "middle", + marginLeft: "4px", + color: theme.custom.colors.silverGrayDark, + cursor: "help", + "& svg": { width: "16px", height: "16px" }, +})) + const CHART_HEIGHT = 320 const COLUMN_FLEX = { @@ -82,24 +92,40 @@ const SERIES = [ column: "active", label: "Active learners", color: CATEGORICAL[0], + /** + * Copied from the `monthly_active_learners` field description in + * ol-analytics-api's b2b_dashboard models (mitodl/ol-analytics-api#57), + * itself derived from the backing dbt SQL in ol-data-platform. That + * description lives only in the OpenAPI schema (/openapi.json, /docs) — + * the actual row data this component fetches never carries it, and this + * client is hand-written rather than generated from the schema (see + * the header comment in analytics/types.ts), so there is no fetch-and- + * parse step that could keep this in sync automatically. If the backend + * description changes, this string has to be updated by hand to match. + */ + description: + "Learners who did anything in a course this month: watched a video, attempted a problem, posted in a discussion, used the chatbot, moved through course pages or earned a certificate. Enrolling alone doesn't count. If too few learners were active, the whole month is withheld to avoid identifying them.", }, { key: "new_enrollments", column: "enrollments", label: "New enrollments", color: CATEGORICAL[1], + description: undefined, }, { key: "certificates_earned", column: "certificates", label: "Certificates earned", color: CATEGORICAL[2], + description: undefined, }, ] as const satisfies ReadonlyArray<{ key: keyof MonthlyEngagementTrend column: keyof typeof COLUMN_FLEX label: string color: string + description?: string }> const EngagementTrendChart: React.FC<{ @@ -152,7 +178,16 @@ const EngagementTrendChart: React.FC<{
{series.label} + {series.description ? ( + + {/* eslint-disable-next-line styled-components-a11y/no-noninteractive-tabindex */} + + + + ) : null} ))} diff --git a/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx b/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx index a394a42376..9b089d935e 100644 --- a/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx @@ -144,6 +144,19 @@ describe("EngagementTrendChart", () => { ).toBeInTheDocument() }) + /** + * "Active learners" alone doesn't say what counts as active — the hover + * icon's accessible label carries the definition for keyboard and screen + * reader users, not just pointer hover. + */ + test("explains what counts as an active learner from the column header", () => { + renderWithTheme() + + expect( + screen.getByLabelText(/Learners who did anything in a course this month/), + ).toBeInTheDocument() + }) + test("shows an empty state rather than an empty chart", () => { renderWithTheme() expect( From 8562830095c9d35eaeead3d3ca9ef4c27d0cec23 Mon Sep 17 00:00:00 2001 From: Danielle Frappier Date: Wed, 23 Sep 2026 09:58:07 -0400 Subject: [PATCH 2/2] Expose active-learner definition on mobile, test the tooltip itself TableHeaderRow (and the hover-definition trigger inside it) is hidden below the md breakpoint, so the definition was unreachable on mobile/tablet. Adds one non-repeating copy outside the table for those widths. Also replaces the aria-label-only test with one that drives real hover and asserts the rendered tooltip, since the old test passed even with Tooltip removed entirely. Addresses Copilot review feedback on PR #3966. Co-Authored-By: Claude Sonnet 5 --- .../Analytics/EngagementTrendChart.tsx | 35 ++++++++++- .../DashboardPage/Analytics/charts.test.tsx | 63 ++++++++++++++++++- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx b/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx index 1464b4b3a0..498b99d657 100644 --- a/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx +++ b/frontends/main/src/app-pages/DashboardPage/Analytics/EngagementTrendChart.tsx @@ -69,11 +69,30 @@ const TableWrapper = styled.div(({ theme }) => ({ const InfoTrigger = styled.span(({ theme }) => ({ display: "inline-flex", + alignItems: "center", verticalAlign: "middle", marginLeft: "4px", color: theme.custom.colors.silverGrayDark, cursor: "help", - "& svg": { width: "16px", height: "16px" }, + "& svg": { width: "16px", height: "16px", marginLeft: "4px" }, +})) + +/** + * The desktop header housing the same trigger is `display: none` below `md` + * (`TableHeaderRow`), so this repeats it once, outside the table, in the one + * layout where that header is hidden — never per row, which would turn one + * definition into a repeated focus stop for every month. + */ +const MobileColumnHelp = styled.div(({ theme }) => ({ + display: "none", + [theme.breakpoints.down("md")]: { + display: "flex", + flexWrap: "wrap", + gap: "16px", + marginBottom: "12px", + ...theme.typography.subtitle2, + color: theme.custom.colors.black, + }, })) const CHART_HEIGHT = 320 @@ -236,6 +255,20 @@ const EngagementTrendChart: React.FC<{ />
+ + {SERIES.filter((series) => series.description).map((series) => ( + + {/* eslint-disable-next-line styled-components-a11y/no-noninteractive-tabindex */} + + {series.label} + + + ))} +
diff --git a/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx b/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx index 9b089d935e..d315010527 100644 --- a/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/Analytics/charts.test.tsx @@ -1,5 +1,11 @@ import React from "react" -import { render, screen, within } from "@testing-library/react" +import { + render, + screen, + waitForElementToBeRemoved, + within, +} from "@testing-library/react" +import userEvent from "@testing-library/user-event" import { ThemeProvider, useTheme } from "ol-components" import type { Theme } from "ol-components" import { factories } from "api/analytics-test-utils" @@ -147,16 +153,67 @@ describe("EngagementTrendChart", () => { /** * "Active learners" alone doesn't say what counts as active — the hover * icon's accessible label carries the definition for keyboard and screen - * reader users, not just pointer hover. + * reader users, not just pointer hover. Scoped to the table: `SERIES` with + * the trigger's description also drives a second, mobile-only copy of this + * same trigger outside the table (see the test below), so an unscoped query + * would match both. */ test("explains what counts as an active learner from the column header", () => { renderWithTheme() + const table = screen.getByRole("table", { name: "Monthly engagement" }) expect( - screen.getByLabelText(/Learners who did anything in a course this month/), + within(table).getByLabelText( + /Learners who did anything in a course this month/, + ), ).toBeInTheDocument() }) + /** + * The column header carrying that trigger is hidden below the `md` + * breakpoint (`TableHeaderRow`), so mobile/tablet users need an equivalent + * — once, outside the table, not repeated for every month's row. + */ + test("repeats the active-learner definition once for mobile, outside the table", () => { + renderWithTheme() + + const table = screen.getByRole("table", { name: "Monthly engagement" }) + const triggers = screen.getAllByLabelText( + /Learners who did anything in a course this month/, + ) + expect(triggers).toHaveLength(2) + expect(triggers.some((trigger) => !table.contains(trigger))).toBe(true) + }) + + /** + * A static aria-label proves nothing about whether the `Tooltip` itself + * works — this test would still pass if `Tooltip` were deleted entirely. + * Driving real hover and asserting the rendered popper text catches that; + * keyboard focus isn't asserted here because MUI only opens on focus when + * `:focus-visible` matches (`isFocusVisible`), which this test environment + * doesn't set for a programmatic `.focus()` call — see the same caveat in + * ContractAdminPage.test.tsx's tooltip test. + */ + test("shows the active-learner definition on hover", async () => { + const user = userEvent.setup() + renderWithTheme() + + const table = screen.getByRole("table", { name: "Monthly engagement" }) + const trigger = within(table).getByLabelText( + /Learners who did anything in a course this month/, + ) + + await user.hover(trigger) + expect( + await screen.findByRole("tooltip", { + name: /Learners who did anything in a course this month/, + }), + ).toBeInTheDocument() + + await user.unhover(trigger) + await waitForElementToBeRemoved(() => screen.queryByRole("tooltip")) + }) + test("shows an empty state rather than an empty chart", () => { renderWithTheme() expect(