feat: add hover explanation for Active learners, fix clipped month label in engagement chart - #3966
Merged
daniellefrappier18 merged 3 commits intoSep 23, 2026
Conversation
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The explanation is unavailable in the mobile layout, and its tooltip interaction is not exercised by the test.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds explanatory help for active learners and prevents the final engagement-chart month label from being clipped.
Changes:
- Adds an accessible tooltip defining active learners.
- Increases chart margin to preserve the final x-axis label.
- Adds tooltip-label coverage.
| File | Description |
|---|---|
EngagementTrendChart.tsx |
Adds the tooltip and adjusts chart spacing. |
charts.test.tsx |
Tests the active-learner explanation label. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alexfigtree
self-requested a review
September 22, 2026 13:39
alexfigtree
approved these changes
Sep 22, 2026
alexfigtree
left a comment
alexfigtree
left a comment
Contributor
There was a problem hiding this comment.
Checked off testing boxes - everything looks good on my end.
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 <noreply@anthropic.com>
daniellefrappier18
deleted the
daniellef/add-hover-description-to-analytics
branch
September 23, 2026 17:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
N/A — no mit-learn issue tracks this directly.
Description (What does it do?)
Two small, related fixes to the "Monthly engagement" table in the B2B analytics dashboard.
Hover explanation for "Active learners". The column header now shows an info icon; hovering or focusing it explains what counts as active. The copy is sourced from the
monthly_active_learnersfield description added in mitodl/ol-analytics-api#57, itself derived from the backing dbt SQL — so the tooltip stays grounded in the same definition that backs the API's own schema. But, that description only lives in the API's OpenAPI schema today, so this is a hand-copied string with a comment explaining why it can drift and needs a manual update if the backend description ever changes.Fixed the last month's x-axis label rendering empty.
@mui/x-chartscaps a point-scale tick's label width at2 * min(space to its left, space toits right). The last tick sits exactly at the drawing area's edge, so that budget collapsed to2 * margin.right— at the previous8pxmargin, only 16px, too narrow for any 3-letter month abbreviation. The library's own ellipsis logic was silently shortening it down to nothing. Bumped the right margin to20px.Screenshots (if appropriate):
How can this be tested?
This assumes local dev on the Tilt/k3d stack (
~/Desktop/work/ol-infrastructure), notdocker compose.Prerequisite: mitxonline manager access — a mitxonline user with
is_manager=Trueon some org, so the dashboard route resolves at all.1. Confirm the analytics-api stub is running. This page's data comes from a local-dev-only stub (the real
ol-analytics-apireads dbt-materialized views out of StarRocks, not realistic to run in k3d), not from anything specific you set up in mitxonline — same stub set up for #3958. It lives onmitodl/ol-infrastructure#5788 (
daniellef/analytics-api-stub); check it out andtilt updeploys it automatically.Additional Context