Skip to content

Redesign concept pages #1346

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 7 commits into from
Mar 19, 2025
Merged

Redesign concept pages #1346

merged 7 commits into from
Mar 19, 2025

Conversation

jacbn
Copy link
Contributor

@jacbn jacbn commented Mar 13, 2025

Applies design changes to concept pages.

Note that some changes to concept pages have already been merged owing to the context overhaul PR being based off the first half of this commit. The changes here are an extension of these, cleaning up odd bits on the page.

Note that the a-link lightbulb icons do not all work in dev, as links to content pages are not seen as local if they start https://isaacphysics.org/, as some do. cp_ang_eq_of_motion has correct links if you need a page to test.

jacbn added 3 commits March 13, 2025 09:55
Fix spacing issues, incorrect count for subjects, and All Filters checkbox when no fields exist
Note that this only works on live! `localLink` is false on dev as the content links to the live site.
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 20.58824% with 27 lines in your changes missing coverage. Please review.

Project coverage is 36.18%. Comparing base (febc870) to head (d71d879).
Report is 67 commits behind head on redesign-2024.

Files with missing lines Patch % Lines
...c/app/components/elements/layout/SidebarLayout.tsx 0.00% 12 Missing ⚠️
src/app/components/pages/Concept.tsx 14.28% 6 Missing ⚠️
src/app/components/elements/ShareLink.tsx 62.50% 3 Missing ⚠️
src/app/components/elements/PrintButton.tsx 0.00% 2 Missing ⚠️
...pp/components/elements/markup/markdownRendering.ts 0.00% 2 Missing ⚠️
src/app/components/pages/Concepts.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           redesign-2024    #1346      +/-   ##
=================================================
- Coverage          36.35%   36.18%   -0.17%     
=================================================
  Files                476      476              
  Lines              21186    21321     +135     
  Branches            7019     6341     -678     
=================================================
+ Hits                7702     7716      +14     
- Misses             12826    13566     +740     
+ Partials             658       39     -619     

☔ 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.

@@ -1,5 +1,5 @@
import React, {useEffect, useRef, useState} from "react";
import {isMobile, isPhy, isTutorOrAbove, PATHS, siteSpecific, useOutsideCallback} from "../../services";
import {isAda, isMobile, isPhy, isTutorOrAbove, PATHS, siteSpecific, useOutsideCallback} from "../../services";

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused import isAda.

Copilot Autofix

AI 3 months ago

To fix the problem, we need to remove the unused import isAda from the import statement on line 2. This will clean up the code and potentially improve performance by reducing the bundle size.

  • In general terms, the problem can be fixed by identifying and removing any unused imports.
  • Specifically, we will remove isAda from the import statement on line 2 in the file src/app/components/elements/ShareLink.tsx.
  • No additional methods, imports, or definitions are needed to implement this change.
Suggested changeset 1
src/app/components/elements/ShareLink.tsx

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/app/components/elements/ShareLink.tsx b/src/app/components/elements/ShareLink.tsx
--- a/src/app/components/elements/ShareLink.tsx
+++ b/src/app/components/elements/ShareLink.tsx
@@ -1,3 +1,3 @@
 import React, {useEffect, useRef, useState} from "react";
-import {isAda, isMobile, isPhy, isTutorOrAbove, PATHS, siteSpecific, useOutsideCallback} from "../../services";
+import {isMobile, isPhy, isTutorOrAbove, PATHS, siteSpecific, useOutsideCallback} from "../../services";
 import {selectors, useAppSelector} from "../../state";
EOF
@@ -1,3 +1,3 @@
import React, {useEffect, useRef, useState} from "react";
import {isAda, isMobile, isPhy, isTutorOrAbove, PATHS, siteSpecific, useOutsideCallback} from "../../services";
import {isMobile, isPhy, isTutorOrAbove, PATHS, siteSpecific, useOutsideCallback} from "../../services";
import {selectors, useAppSelector} from "../../state";
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
@sjd210
Copy link
Contributor

sjd210 commented Mar 18, 2025

This generic concepts section of SidebarLayout (I would comment on it directly, but I cant here since it was merged as part of the Previous Page Context PR) has slightly different behaviour to the HierarchyFilter in the Question Finder sidebar.

In particular when a subject checkbox is a parent of a selected checkbox (and therefore isPartial) here and is clicked, the parent remains selected while the children become deselected. On the QF, the parent AND children become deselected. I prefer this latter behaviour, but either way it should at least be consistent.

@sjd210
Copy link
Contributor

sjd210 commented Mar 18, 2025

Are we abandoning the other parts of the concept sidebar from the designs? Or perhaps handling it later (in a separate card from the listings page)? Waiting for something from content?

i.e. the following:

  • Splitting related questions up into current context's learning stage and other learning stages
  • Linking to "All GCSE Concepts/Questions"
  • Indicating whether questions have been attempted/completed

@jacbn
Copy link
Contributor Author

jacbn commented Mar 18, 2025

  • Splitting related questions up into current context's learning stage and other learning stages
  • Linking to "All GCSE Concepts/Questions"

We cannot do either of these until the concept pages have been tagged with an audience. At the moment, we cannot infer what context the concept page is if we land on it directly, and even if we come through e.g. GCSE Physics, the hook that uses the previous context doesn't work without the page being landed on being tagged (needed to check whether the page is still applicable to GCSE Physics).

There was also a discussion on how the page context interacts with the user's context, which somewhat muddies the question of what stage the "All X Concepts" should display. The result of that was that page context should always take priority, fwiw.

  • Indicating whether questions have been attempted/completed

Related questions for concept pages are not currently being augmented in the API. This'll need to be a separate change, and shouldn't touch any of this code, so perhaps we should make a separate card for this.

@jacbn
Copy link
Contributor Author

jacbn commented Mar 18, 2025

the parent remains selected while the children become deselected

Ah, good spot. When you select a child, the parent tag is removed as the search here is an OR, so everything in the entire subject would otherwise be searched. So when you click the parent, it's currently in an off state, so switches on. Will fix.

@sjd210
Copy link
Contributor

sjd210 commented Mar 19, 2025

We cannot do either of these until the concept pages have been tagged with an audience

Fair enough - I figured it'd be something along these lines. I'd just wanted to know what was going on specifically here wrt these changes.

(Also to note it here: This is good - just waiting for release to merge)

@sjd210 sjd210 merged commit d9ba680 into redesign-2024 Mar 19, 2025
8 checks passed
@sjd210 sjd210 deleted the redesign/concept-pages branch March 19, 2025 17:18
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