Deck builder rework + bookmarks integration#2149
Conversation
Tidies up some interactions between `.btn` and `.icon`, allowing for much more variation in how icon colours respond to hover. Also introduces a blank button type, with relevant icon support.
Allows better distinguishing from the ALVI-based row, and will enable correct dep splitting when complete
(all feature flagged!)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2149 +/- ##
==========================================
- Coverage 43.68% 43.52% -0.16%
==========================================
Files 594 597 +3
Lines 25043 25158 +115
Branches 7425 8331 +906
==========================================
+ Hits 10940 10951 +11
- Misses 14054 14151 +97
- Partials 49 56 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…eature/gameboard-builder-rework
sjd210
left a comment
sjd210
left a comment
There was a problem hiding this comment.
I haven't looked at everything in depth, but I've already found quite a lot that I think is unusual so I'm going to hand it back with these comments and look more closely at the code itself once things have changed for a re-review.
I think the general dragging structure and ALVI layout is looking good, but some edge cases were missed - and I'd like to have more of a discussion about how filtering is going to work when searching with books, because I don't like our old system and think we can use the opportunity to improve upon it.
| </Row> | ||
|
|
||
| <Row> | ||
| <Col className={classNames("mb-2", {"d-none": isBookSearch})}> |
There was a problem hiding this comment.
In a similar area to my last comment, and following the "consider the possibility of having book sections appear in the sidebar, in place of the topic selection" point on the original card, I still find the existing system of hiding other options when a book is selected incredibly odd.
The original reason we did this (I believe?) was so we could provide a list of all questions in book order, when titles were used programmatically to accommodate this. We don't structure titles like this anymore, so this reason is void.
The reason we might want to do this now is because not all topics/stages are able to apply to a given book, but this doesn't accomodate books that DO cover many topics/stages, or for searching by difficulty, and it's also just inconsistent with the style of searching that we already use on the question finder - I think this is actively confusing. For example as a teacher, it would frustrate me if I wanted to look for Further A appropriate content in Pre-Uni Maths, but the option is hidden from me.
The API is suited just fine to searching with both books and other terms (hence the search already including those other terms as per my last comment), and I really do think that the simplest thing for now would be enabling all these options. (I think allowing searching by book section IS a nice idea, but there's no reason we need to keep these other restrictions while waiting for that to be implemented).
There was a problem hiding this comment.
The original reason we did this (I believe?) was...
It's more complicated than this, as always. We did this because the backend questionSearch function was a big mess.
We used map all kinds of tags together into one big clump – topics, books, subject etc were all just "tags". These tags, when thrown into the content manager search, were combined in a single "OR" search. Books being included in this OR was a pain; if you added other tags like things inside maths, you'd end up with a slew of maths results mixed in with the book results. To fix this, we just disabled searches while books were set, so you only saw that book.
Anyway, some work was done on the QF a while back to distinguish different tag types by expanding the /questions endpoint. Each tag type is now ORed amongst itself, but ANDed across each type (i.e. a search for book1, book2, topic1, topic2 results in questions that match (book1 || book2) && (topic1 || topic2)
We've had this functionality on the QF for a while, but apparently it didn't migrate to the builder. We need to move the books from the tags field to a new books field to achieve this.
Having done this, I will say that 95% of topics not applying to a given book feels quite annoying from a UX perspective. Grouping by chapter is the only request we've had from teachers about this filtering and would be so much better than this. I've copied over the filter list from the top of the QF to hopefully make it more visible when there are filters "blocking" questions from appearing to make it less egregious.
This is a little ugly, but the `<li/>`s *require* a white background (so as to appear as such while dragging), which necessarily overlaps any outline I can give it.
I don't love having to remove the drag handle, but we're really limited on space. A typical mobile app would use left/right swiping for certain actions (e.g. removal) which would help, but that's a significant amount of extra work.
sjd210
left a comment
sjd210
left a comment
There was a problem hiding this comment.
A few more things to fix, but nothing major.
On the VRT front, the slight pixel chanes do appear to also be happening for me locally in this branch but not elsewhere - I'm not sure what's causing it.
| </Row> | ||
|
|
||
| <Row> | ||
| <Col xs={6} lg={4} className={classNames("mb-2")}> |
There was a problem hiding this comment.
The Ada-specific Exam Board select is leaving a gap in this row for Isaac
| <Col xs={6} lg={4} className={classNames("mb-2")}> | |
| <Col xs={6} lg={siteSpecific(6,4)} className={classNames("mb-2")}> |
| options={getFilteredStageOptions()} onChange={selectOnChange(setSearchStages, true)} | ||
| /> | ||
| </Col> | ||
| <Col xs={6} lg={4} className={classNames("mb-2")}> |
There was a problem hiding this comment.
See above
| <Col xs={6} lg={4} className={classNames("mb-2")}> | |
| <Col xs={6} lg={siteSpecific(6,4)} className={classNames("mb-2")}> |
|
|
||
| <Col className="col-12 col-xl-9"> | ||
|
|
||
| <FilterSummary filterTags={filterTags} removeFilterTag={removeFilterTag} clearFilters={clearFilters} /> |
There was a problem hiding this comment.
There being filter options in the sidebar for FastTrack and bookmarked questions, but them not showing up in this summary is odd (especially since I would expect "Clear all filters" to actually clear every filter). Can we add them as filter tags?
| // subtitle={item.subtitle} | ||
| // breadcrumb={breadcrumb} |
There was a problem hiding this comment.
Do we need these commented lines around? I think these look fine without the subtitle/breadcrumb fwiw.
| } | ||
|
|
||
| .modal-body { | ||
| overflow: auto; |
There was a problem hiding this comment.
Without this overflow: auto, the title graphics on a question preview displays beyond the bounds of the modal.
| border-top-left-radius: 0; | ||
| border-bottom-left-radius: 0; | ||
| padding: 0.625rem; | ||
| min-width: 36px; |
There was a problem hiding this comment.
The Ada version of this button now has a similar issue that we solved here for Isaac (the button is shrinking from the right from ~400px). Could we add a similar min-width for it too?
2 participants
Something is wrong with VRTs as the moment – still working out whether it's caused by this branch or not. The code is perfectly reviewable however :)
Reworks the deck builder to:
TODO: checkall good!@hello-pangea/dndis not in the main bundle!