-
Notifications
You must be signed in to change notification settings - Fork 716
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
EQM: Sections for quiz detail page #12384
EQM: Sections for quiz detail page #12384
Conversation
@@ -153,8 +153,8 @@ | |||
displaySectionTitle, | |||
enhancedQuizManagementStrings, | |||
} from 'kolibri-common/strings/enhancedQuizManagementStrings'; | |||
import { coreStrings } from 'kolibri.coreVue.mixins.commonCoreStrings'; |
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.
Can't use import coreStrings from 'kolibri.utils.coreStrings
from here
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.
This is outdated but I get errors on my end locally without changing the import here.
Build Artifacts
|
.question-button { | ||
width: 100%; | ||
height: 100%; | ||
padding: 0.5em; | ||
|
||
&:hover { | ||
background-color: white; | ||
} | ||
|
||
&.selected { | ||
background-color: white; | ||
} | ||
} |
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.
A bit of a stop-gap, just wanted to get this PR made, will include notes on this in a tech-debt follow-up
import AccordionItem from 'kolibri-common/components/AccordionItem'; | ||
import AccordionContainer from 'kolibri-common/components/AccordionContainer'; | ||
import coreStrings from 'kolibri.utils.coreStrings'; | ||
import { annotateSections } from 'kolibri.utils.exams'; |
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.
I appreciate this. I was thinking something along these lines earlier today but decided to just barrel ahead to get things done. If only I'd rebased earlier😭
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.
Code wise, this all makes sense. No manual testing yet.
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.
The basic functionality here seems to work in manual QA, although (as we discussed on slack) it's not really an ideal experience with the layout. I think that's okay for now and it's worth merging in for Radina and Peter to do more robust testing and see if they have any ideas of small improvements we could make in the short term.
Otherwise, I only have one notable probably-blocker. I am testing with the dev tools not on actual mobile device, but what I'm noticing is that the KSelect goes up and that you can't scroll within it quite all the way? There's some amount of scrolling that works, but some of the question items are cut off, and even when I do scroll, I can't get to those questions < 4 depending on the position of the popover. It may be a pre-existing issue to KSelect and this is just an odd manifestation of it, but I think it's worth at least like a "is this fixable in 30 minutes" troubleshooting, if possible. If it's a KDS level problem, it doesn't have to block this PR
scrolling.mp4
Created a follow up issue for the KSelect related problem, somewhat out of scope
Summary
Updates the quiz preview/details page to allow for quiz question navigation w/ sections.
Imitates the mobile experience made for reports, but simplified as the UI is only for navigation.
This results in an accordion on > small screens, and two KSelects on small screens.
Tech Debt
There is a need to extract some of this logic out and improve the architecture of how we use accordions altogether and how we manage sections & questions for UI and navigation. This introduces some duplicate code but, along with some improvements to the Accordion components altogether.
Comments & suggestions here are greatly welcome, as I'll create a follow-up issue to this effect.
References
Fixes #12288
Reviewer guidance
Please see and test:
For: