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
Enable ProgressBar for quizzes #835
Enable ProgressBar for quizzes #835
Conversation
|
|
|
|
|
|
|
|
const [isQuiz, setIsQuiz] = useState(false); | ||
|
||
const quizCompleted = useMemo(() => { | ||
return quiz.currentQuestion === quiz?.questionCount; |
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.
Does the first half of this need a guard against quiz
being blank as well?
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.
what happens if both sides are null/undefined?
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've added optional chaining to the first half.
|
describe("When on a quiz step", () => { | ||
beforeEach(() => { | ||
renderProgressBarOnStep(2); | ||
}); | ||
|
||
test("Progress bar renders with correct max and value", () => { | ||
const progressBar = screen.getByRole("progressbar"); | ||
expect(progressBar).toHaveValue(2); | ||
expect(progressBar).toHaveAttribute("max", "3"); | ||
}); | ||
|
||
test("Clicking previous step button goes back a step", () => { | ||
const previousStepButton = screen.getByTitle( | ||
"instructionsPanel.previousStep", | ||
); | ||
fireEvent.click(previousStepButton); | ||
expect(store.getActions()).toEqual([setCurrentStepPosition(1)]); | ||
}); | ||
|
||
test("Clicking next step button goes forward a step", () => { | ||
const nextStepButton = screen.getByTitle("instructionsPanel.nextStep"); | ||
fireEvent.click(nextStepButton); | ||
expect(store.getActions()).toEqual([setCurrentStepPosition(3)]); | ||
}); |
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 sure I quite understand what's going on here... I might be missing something, but what indicates that the "Quiz step 1" step is a quiz?
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.
Yes you're right! I've fixed (I think I reverted some changes I made and removed the knowledgeQuiz property on that step). Thanks for spotting!
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'm still not sure I understand... Does the progress bar know anything about whether it's a quiz?
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.
No it doesn't.
I put it in since the ProgressBar was previously removed on a quiz step but maybe this validation is unnecessary
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.
Hmmm yeah, tbh I was wondering whether the test was redundant 🤔 I guess in another file you've got the test that the project bar shows on a quiz step, so I think that's enough... I suppose all you've done is remove the logic to hide the progress bar on a quiz step, so if the tests were to go back to how they were before that was added, that would make sense in my mind at least 😅
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 - I've removed!
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.
Added a few comments on this one and looks like you're missing a changelog entry too
|
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.
Looks good to me 👍
|
closes #834
quizReady
state