-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
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>
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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.
…-react-app into redesign/previous-context-overhaul
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:
pageContext
management should come from a hook defined inpageContext.ts
. As of this PR, there are two;useUrlPageTheme
for pages where the URL is the source of truth, andusePreviousPageContext
for pages where the previous page's context should be maintained if possible.useEffect
s inside these functions). That is,pageContext
'ssubject
andstage
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.previousContext
, topageContext
. 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.resetIfNotFound
parameter onuseUrlPageTheme
, as the context is now always between page transitions.