Skip to content

Redesign: practice tests #1402

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

Open
wants to merge 25 commits into
base: redesign-2024
Choose a base branch
from
Open

Conversation

axlewin
Copy link
Contributor

@axlewin axlewin commented Apr 16, 2025

  • Restyle practice test listing pages to match designs
  • Add filtering to subject/stage-specific practice test pages

@axlewin
Copy link
Contributor Author

axlewin commented Apr 16, 2025

A few points/questions:

  • The designs have "showing x of y" text and a "load more" button, but we currently fetch all practice tests at once. Considering how few of them there are I don't think this is worth changing.
  • The filters work as radio buttons. For the topic selection, multi-select might be more useful?
  • The Preview button doesn't currently show for students, but could link to the quiz rubric once those changes are merged.
  • Several of the subject/stage-specific pages are empty - we could redirect to the main practice tests page in this case?

Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 18.24818% with 112 lines in your changes missing coverage. Please review.

Project coverage is 40.76%. Comparing base (a6826ff) to head (fb428be).

Files with missing lines Patch % Lines
...c/app/components/pages/quizzes/PracticeQuizzes.tsx 14.03% 48 Missing and 1 partial ⚠️
...c/app/components/elements/layout/SidebarLayout.tsx 6.25% 45 Missing ⚠️
...c/app/components/elements/list-groups/ListView.tsx 47.61% 11 Missing ⚠️
...ents/elements/list-groups/AbstractListViewItem.tsx 36.36% 7 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           redesign-2024    #1402      +/-   ##
=================================================
- Coverage          40.90%   40.76%   -0.14%     
=================================================
  Files                493      493              
  Lines              21871    21950      +79     
  Branches            6453     7260     +807     
=================================================
+ Hits                8947     8949       +2     
+ Misses             12886    12382     -504     
- Partials              38      619     +581     

☔ 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

@jacbn jacbn left a comment

Choose a reason for hiding this comment

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

There is a big simplification (admittedly only at a glance, there could be more nuance I've missed) to be made by reusing <ListView/>, which also keeps this in line styling-wise with everything else. We could also add more sections/features to the sidebar if we want, but there aren't a huge number of tests atm so the value we get from these is a little diminished. Will ask content what they want from this when we're back.

<ListGroup className="mb-3 quiz-list">
{quizzes.filter((quiz) => showQuiz(quiz) && quiz.title?.toLowerCase().includes(filterText.toLowerCase())).map(quiz => <ListGroupItem className="p-0 bg-transparent" key={quiz.id}>
<div className="d-flex flex-grow-1 flex-column flex-sm-row align-items-center p-3">
{quizzes.filter((quiz) => isRelevant(quiz)).length > 0 && <ListGroup className={siteSpecific("list-results-container p-2 my-4", "mb-3")}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be reusing ListView here?

This entire block can be replaced with just

<ListView items={quizzes.filter((quiz) => isRelevant(quiz))}/>

and if any styling needs to change (e.g. for Ada) we should do it on the ListView components.

Having said that, I'm not 100% sure the "should this button be displayed" logic is identical between the two. Can take a closer look when we're back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I hadn't realised we were using ListViews for Ada too - that simplifies this nicely (although as you mention, Ada will need some restyling).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is less straightforward now that we're only showing one "View Test" button for practice tests. We could try to distinguish between practice & non-practice tests within QuizListViewItem, but I'm not sure that's any neater than the current implementation.

{Subjects.filter(s => subjectCounts[s] > 0).map((subject, i) =>
<li key={i}>
<StyledTabPicker checkboxTitle={HUMAN_SUBJECTS[subject]} checked={filterSubject === subject}
count={subjectCounts[subject]} onInputChange={() => {setFilterSubject(subject);}}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
count={subjectCounts[subject]} onInputChange={() => {setFilterSubject(subject);}}/>
count={subjectCounts[subject]} onInputChange={() => {setFilterSubject(subject);}}
data-bs-theme={subject}
/>

I am a bit of a sucker for colours here :)

Though, I wonder if it is worth having a QF-style selector which expands into fields after you select a subject, so you can do everything you can do in subject-specific, in the generic one? I'm not convinced there are enough tests for it to be useful at the moment, but if it turns out to be a copy-paste job from the QF I'd be tempted to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, would a stage selector be useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try out both of these next week. There is the risk of highlighting how few tests there are if most filter combinations return nothing, but that's the case for the subject-specific pages anyway so I think it's fine.

axlewin and others added 2 commits April 24, 2025 16:07
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
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.

3 participants