-
Notifications
You must be signed in to change notification settings - Fork 687
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
Finishing up the Quiz Root Page #11434
Finishing up the Quiz Root Page #11434
Conversation
Build Artifacts
|
First of all, congrats to you and @AllanOXDi for all the work on this so far! Great progress, this is starting to look and feel almost exactly as I hoped from the designs! 👏🏽 💯 Keyboard navigation is logical and even though it will get more complex once questions and answers are in play, so far it's not overwhelming. There are just a few rough edges remaining to smooth. Tabs for sectionsWork as expected, according to our KDS component, a couple of notes:
Accordion for questionsA bit more polishing needed here. First of all the missing labels:
Now the interaction:
Again, great job so far, we're almost there! 🙂 |
…bs-with-overflow Merging this to mitigate rebase issues; @LianaHarris360 ' review notes will be handled in #11434 which is based on this PR
52ef0cf
to
8af8d74
Compare
e5b805c
to
ebc7ac5
Compare
ebc7ac5
to
b49fa31
Compare
b49fa31
to
4e760ec
Compare
Tests will be fixed with rebase on develop after merging #11535 |
9ce8840
to
4aa72d2
Compare
Ran into some issues w/ the way the overflow calculation was done as I tried to adjust the heights of the tabs and their containers themselves This ultimately ends up losing some items which were impacting the calculations which will be fixed in a subsequent commit
KDS commit 047379f579ad7067381a9576edc21abd12ad8378 is a req for this change
4aa72d2
to
51d0e6a
Compare
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.
Thanks @nucleogenesis. Following our live review, I have just two comments. One is non-blocking but a response/little discussion would be great just to have a reference for it in github somewhere. the other is a very tiny cleanup request. Overall I think it will be okay to merge and build on top of this. Thanks for all the work here
@@ -64,6 +99,7 @@ export default () => { | |||
* @throws {TypeError} if section is not a valid QuizSection | |||
**/ | |||
function updateSection({ section_id, ...updates }) { | |||
console.log('updating with...', section_id, updates); |
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.
very small but can you either get rid of this or add a TODO to cleanup console logs later?
@@ -2,6 +2,9 @@ | |||
|
|||
<div> | |||
<h1>Replace questions</h1> | |||
<p v-for="(question,index) in quizForge.selectedActiveQuestions.value" :key="`${index}q`"> | |||
{{ JSON.stringify(question) }} |
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.
not blocking: the JSON.stringify(question)
here feels very weird. is there something about the data structure that requires this, whereas in other places we don't need it? Is it permanent or is this because of the debugging setup? I know that we do use JSON.stringify in places but I am struggling to think of other instances where we use it in a vue template.
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 will be wiped out in #11497 -- I just put it there as an example of like... "look here is where/how you get the questions"
Merge learningequality/kolibri-design-system#497 before merging this
Summary
useQuizCreation
module that populates some basic data for quick visual testingThis is based directly on the work laid out in #11382 -- commits up to the "KDS Linkage" commit are not new work in this PR and will be removed when that other PR merges to develop and this is rebased on that.
References
#11382
#11013
Reviewer guidance
TBD
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)