Skip to content

Previous page context overhaul #1341

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.

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

Merged
merged 13 commits into from
Mar 12, 2025

Conversation

jacbn
Copy link
Contributor

@jacbn jacbn commented Mar 10, 2025

This is dependent on the first few commits of the concept page redesign, which isn't finished yet; there will be some harmless but unrelated changes in Concepts.tsx if this is reviewed first (I wouldn't wait on the concepts PR though, this is otherwise entirely separate).


In the redesigns, there is an idea of a "previous context", whereby pages have some recognition of how the user got to this point. This is primarily used for the breadcrumb, on pages where the URL is not the ground truth for the page context (e.g. question pages). On these, we would like a link back to the subject/stage landing page if that was where we came from, so that the user journey is more clear.

We managed this before by simply not updating the context between page switches; if you were previously on the A Level Chem landing page, this would be the initial context on the next page load, and this could be compared with the doc properties if loading a e.g. question page to see whether this was still valid or whether it should change. However, a problem with this approach is that we are relying on the page we are visiting to continue managing the context. This would mean needing context-related code on all pages to work properly; otherwise, we would have (as we do currently) a problem where context is not updated on e.g. the homepage (which should be neutral, but does not have any code to "reset" the context), which causes e.g. the navbar at the top, which itself uses the context as a source of truth, to be incorrectly coloured.

The new approach proposed in this PR provides several changes to this method:

  • All pageContext management should come from a hook defined in pageContext.ts. As of this PR, there are two; useUrlPageTheme for pages where the URL is the source of truth, and usePreviousPageContext for pages where the previous page's context should be maintained if possible.
  • All hooks must manage both setup and teardown of the context on the relevant page (see the return statements in the useEffects inside these functions). That is, pageContext's subject and stage fields must both always be reset on leaving a page. This ensures we do not need any context management code on non-question, non-concept pages, which as we have a lot of hard-coded generic pages, is extremely useful.
  • This necessarily required the addition of a new field, previousContext, to pageContext. This should always store the context that was reset during teardown. This can then be reused in e.g. usePreviousPageContext to check if this context is still valid.
  • This simplification has also removed the need for the resetIfNotFound parameter on useUrlPageTheme, as the context is now always between page transitions.

jacbn added 8 commits March 5, 2025 15:39
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.
When there are multiple valid themes to choose from, pick the one most relevant to the page context.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Copy link

codecov bot commented Mar 10, 2025

Codecov Report

Attention: Patch coverage is 13.55932% with 102 lines in your changes missing coverage. Please review.

Project coverage is 36.35%. Comparing base (c42a0b0) to head (227fc01).
Report is 16 commits behind head on redesign-2024.

Files with missing lines Patch % Lines
...c/app/components/elements/layout/SidebarLayout.tsx 10.00% 45 Missing ⚠️
src/app/components/pages/Concepts.tsx 3.84% 25 Missing ⚠️
src/app/services/pageContext.ts 22.72% 17 Missing ⚠️
...c/app/components/elements/list-groups/ListView.tsx 25.00% 6 Missing ⚠️
src/app/components/pages/Concept.tsx 50.00% 2 Missing ⚠️
src/app/components/pages/Question.tsx 33.33% 2 Missing ⚠️
src/app/state/slices/context.ts 0.00% 2 Missing ⚠️
...ents/elements/panels/QuestionFinderFilterPanel.tsx 0.00% 1 Missing ⚠️
src/app/components/pages/QuestionFinder.tsx 0.00% 1 Missing ⚠️
src/app/state/selectors.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           redesign-2024    #1341      +/-   ##
=================================================
- Coverage          36.42%   36.35%   -0.08%     
=================================================
  Files                476      476              
  Lines              21142    21186      +44     
  Branches            6263     6288      +25     
=================================================
  Hits                7702     7702              
- Misses             13401    13445      +44     
  Partials              39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be some strange git behaviour around this file...I have a feeling that restoring it now might just cause more issues, so maybe removing it here and replacing it once we update all the phy VRTs is fine.

@axlewin axlewin merged commit febc870 into redesign-2024 Mar 12, 2025
8 checks passed
@axlewin axlewin deleted the redesign/previous-context-overhaul branch March 12, 2025 14:24
@jacbn jacbn mentioned this pull request Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants