Convert outdated ContentSummaryListGroupItems to AbstractListViewItems implementations #1814
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
[VRT] Update baselines for improvement/remove-CSLGIs
(and associated components getProgressIcon, GameboardItemComponent, Wildcard, GameboardViewerInner)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1814 +/- ##
==========================================
+ Coverage 41.56% 41.75% +0.19%
==========================================
Files 543 543
Lines 23784 23689 -95
Branches 7863 7849 -14
==========================================
+ Hits 9885 9891 +6
+ Misses 13256 13157 -99
+ Partials 643 641 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacbn
left a comment
jacbn
left a comment
There was a problem hiding this comment.
Couple of things to fix, but overall this is such a nice piece of debt to get off our backs! Thanks for picking this one up.
src/app/components/elements/list-groups/AbstractListViewItem.tsx
Outdated
Show resolved
Hide resolved
src/app/components/elements/list-groups/AbstractListViewItem.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const icon: TitleIconProps = isPhy | ||
| ? {type: "hex", icon: "icon-question", size: "lg"} | ||
| : item.state === CompletionState.IN_PROGRESS |
There was a problem hiding this comment.
We're missing ALL_ATTEMPTED in this ternary, so it's showing up as not started. We could reuse the in progress one:
| : item.state === CompletionState.IN_PROGRESS | |
| : [CompletionState.IN_PROGRESS, CompletionState.ALL_ATTEMPTED].includes(item.state as CompletionState) |
but it does annoy me that we don't have a function to perform consistent generation of status symbols somewhere. We have similar logic here (different output unfortunately), but that's for physics.
I think the output of this mystical function ought to be:
question status (CompletionState) |
sci symbol | ada symbol |
|---|---|---|
| ALL_CORRECT | status-correct | status-correct |
| ALL_ATTEMPTED | status-attempted | status-in-progress |
| ALL_INCORRECT | status-attempted | status-incorrect |
| IN_PROGRESS | status-in-progress | status-in-progress |
| NOT_ATTEMPTED | status-not-attempted | status-not-attempted |
But note that for question parts, sci uses INCORRECT => status-incorrect:
part status (QuestionPartState) |
sci symbol | ada symbol (currently unused) |
|---|---|---|
| CORRECT | status-correct | status-correct |
| INCORRECT | status-incorrect | status-incorrect |
| NOT_ATTEMPTED | status-not-attempted | status-not-attempted |
I'm not suggesting we do this now (unless you're keen!) as the first suggestion will work fine. But it is something to pre-prep if not, at least.
There was a problem hiding this comment.
I wonder, coming back to what this function might output, if we could supersede type: hex in TitleIconProps with type: icon or similar, which does precisely what hex does but doesn't create the background hex on Ada (or indeed just have 'show hex' be a prop we enable for Sci), and we can use size: "md" for the 24px sizing. That way, all things that use it create a TitleIconProps.
There was a problem hiding this comment.
I've made a start on this (which I've moved to another branch), but there are some complications like Ada not using consistent square icon sizes (see cs/icons/concept with 26x35px for example) which means we can't quite do sizes in the same way without either changing these icon sizes or the surrounding code. It's still doable, just out of scope of this PR - I'll make that card.
Arguably the generic question icon should also be 32px here, but this makes it too big in other places like the QF. This can be reconsidered in the upcoming icon refactor PR, but for now this is consistent with live.
ContentSummaryListGroupItems are a legacy way of dealing with a list of objects (e.g. on the question finder or search page). We have replaced this with the more robust and type-smartAbstractListViewItemduring the Isaac Science redesign, but until now Ada CS was still using this old component.ALVIs are built in a modular(ish) way so this was relatively straightforward to convert, with the following points of significance:
TopicListViewItemfor topics in Ada searchhasCaretas a top-levelListViewprop, allowing for a right-caret/chevron to be displayed to the right of each item in the list, used in Ada search and assignmentsGameboardViewercomponent for similar reasons; it was causing issues such as not displaying the LLM-marked question indicator. This has been replaced everywhere on Ada, and specifically the Assignment Schedule question deck preview on Isaac (which I think was just accidentally missed during redesign)Overall the lists should look nearly identical to before, with exceptions such as including the aforementioned LLM-marked question indicator on Ada quizzes and some minor layout changes at smaller screenwidths (which I think are for the better). If you see anything else in review let me know.