My Ada Sidebar

[jacbn]

A good chunk of these line changes are just whitespace from adjusting the structure of the sidebar-containing component!

To fix the problem, we should remove the unused isPhy named import from the import statement on line 7. In general, unused import issues are resolved by either removing the unused symbol or updating the code to actually use it if it was unintentionally omitted from usage. Here, there is no indication that isPhy should be used, so the minimal, non-functional change is to delete it from the import list.

Concretely, in src/app/components/pages/Concept.tsx, locate the import line starting with import {Subject, usePreviousPageContext, isAda, useNavigation, siteSpecific, useUserViewingContext, isFullyDefinedContext, isSingleStageContext, LEARNING_STAGE_TO_STAGES, isDefined, isPhy} from "../../services"; and remove isPhy from the curly-brace list, leaving the rest of the imports unchanged. No additional methods, imports, or definitions are needed.

To fix an unused import, the safest approach is to remove just the unused identifier from the import statement, leaving the other imports intact. This preserves all existing behaviour while cleaning up dead code.

In this case, in src/app/components/pages/News.tsx, line 8 currently imports isPhy, NEWS_PODS_PER_PAGE, and siteSpecific from "../../services", but only NEWS_PODS_PER_PAGE and siteSpecific are used. The best fix is to edit that line to remove isPhy from the destructuring import. No other code changes, methods, or additional imports are required.

In general, unused imports should be removed to keep the codebase clean and avoid confusion. For a named import that is not referenced anywhere in the file, the safest fix is simply to delete that specific import specifier (or the entire import statement if nothing else is imported from that module).

In this case, the file src/app/components/pages/Programmes.tsx imports isPhy from "../../services" on line 6, and the symbol is not used anywhere in the component. The best fix without changing existing functionality is to remove that import line entirely. No other imports from "../../services" are on that line, and there is already a separate siteSpecific import on line 9, so we do not need to modify or merge anything else. No additional methods, imports, or definitions are required.

In general, unused imports should be removed to keep the codebase clean and avoid confusion. Here, the single best fix is to delete the named import isPhy from the ../../services module in src/app/components/pages/RevisionDetailPage.tsx.

Concretely, in RevisionDetailPage.tsx, remove line 11:

import { isPhy } from "../../services";

No additional code changes are needed because isPhy is not referenced anywhere in this file. We do not need to adjust any other imports or logic, and we do not need any new methods, imports, or definitions to implement this change.

In general, the fix for an unused import is to remove just that symbol from the import list, leaving any actually-used imports intact. This preserves existing functionality while cleaning up dead code and preventing linter or build warnings.

For this file, we should edit src/app/components/pages/Support.tsx and modify the named import from "../../services" so that it no longer includes isPhy. We must keep ifKeyIsEnter, isAda, isDefined, and siteSpecific unchanged, since at least siteSpecific is clearly used (and the others may be used in parts of the file not shown). No new methods, imports, or definitions are needed; this is a one-word removal from the existing import statement.

In general, unused imports should be removed from the import list. This reduces noise, avoids confusion about whether the symbol is used, and can help bundlers/tree‑shakers produce smaller builds.

The best fix here is to edit src/app/state/selectors.tsx so that the import from "../services" only imports NOT_FOUND, removing siteSpecific from the curly‑brace list. This preserves all existing functionality because siteSpecific is never referenced in this file. No other code changes or new imports are required.

Concretely: in src/app/state/selectors.tsx, at the top of the file where you have import {NOT_FOUND, siteSpecific} from "../services";, change it to import {NOT_FOUND} from "../services";. All other lines remain the same.

To fix the problem, remove the unused imported symbol siteSpecific from the import statement while leaving the used isDefined import intact. This will not change any runtime behavior, since unused bindings have no effect, but it will clean up the code and satisfy the static analysis rule.

Concretely, in src/app/components/elements/inputs/StyledTabPicker.tsx, edit the import on line 4. Change import { isDefined, siteSpecific } from "../../../services"; to only import isDefined. No additional methods, imports, or definitions are required.

[jacbn]

This has annoyingly gone through a lot of back-and-forth changes, but I have now moved the working sidebar inside a feature flag, with the intention of being able to just get this merged and future changes being much smaller and easier to review. I have also split the container refactor into its own PR

As is, this PR adds a sidebar to all the My Ada pages. There are a few changes still to make to the teacher side, perhaps the most notable being replacing the My Ada dropdown in the top menu with a button that takes you to the overview. There is also additional work required on the student side, the largest being a student overview page.

With this in mind, the work for the sidebar is obviously incomplete, but for the sake of this review it would be useful to:

• double check that no changes that are intended to be behind a feature flag affect the non-feature flag version;
• review the improved StyledSelect component + CSS;
• review the functionality (not really the contents, as this is subject to change) of the sidebar, on both mobile and standard screens;
• and lastly, I would really appreciate opinions on changing the breakpoint sizes on Ada. I am not convinced what I did here was necessarily the best approach, though a cleaner approach is significantly more work and would need to affect far more pages than the scope of this PR.

As a short summary of this last point, pages were built around md being a particular size; the reason breakpoints exist is so that content doesn't overflow or be noticeably wrapped. If we just add in a sidebar, however, the "effective" size for the page decreases because there is less space available for the page content, despite the screen width still being the same. The real fix for this is to move the entire page layout to containerised widths, i.e. if the content exceeds a breakpoint then it changes layout, rather than the screen. Unfortunately, this is not how Bootstrap works (w-md-50 for example always refers to the screen width) and basically our entire layout system uses Bootstrap. I really would like to make this change at some point, but this needs a lot of work. As a simpler alternative, I have just boosted the Bootstrap breakpoint values to include the expanded sidebar width (~220px). This obviously has drawbacks (pages use smaller breakpoints than they could when the sidebar is collapsed or not present) and is why I wonder if something else could be done. I do think, however, that since the sidebar is relatively small, the impact only affects particular devices, and using a mobile layout "too late" is much better than the other way around of using a desktop layout "too early".