-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add DOJO exercise filtering checkbox #2383
Add DOJO exercise filtering checkbox #2383
Conversation
@bryanjenningz is attempting to deploy a commit to the c0d3-prod Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ Coverage Diff @@
## master #2383 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 184 184
Lines 3239 3270 +31
Branches 856 861 +5
=========================================
+ Hits 3239 3270 +31
|
<Exercise | ||
key={exerciseIndex} |
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 removed key
here because we want the Exercise
component to not rerender since the Exercise
component now holds more state which we don't want to reset by rerendering.
Another way I could have implemented this would be to keep userAnswers
and localUserAnswers
at the same level here and then only pass down localUserAnswers
to the Exercise
component.
Both ways work, but I chose to keep localUserAnswers
at the Exercise
component level.
submitUserAnswer={(userAnswer: string) => { | ||
setUserAnswers({ ...userAnswers, [exercise.id]: userAnswer }) | ||
exercises={currentExercises} | ||
userAnswers={userAnswers} |
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 userAnswers
prop here only gets used to initialize the localUserAnswers
state in the Exercise
component. Maybe a better name for this prop would be initUserAnswers
or initLocalUserAnswers
.
onExit={localUserAnswers => { | ||
setUserAnswers({ ...userAnswers, ...localUserAnswers }) | ||
setSolvingExercise(false) | ||
}} |
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.
onExit
gets called when we exit the Exercise
component and go back to the ExerciseList
view mode. The userAnswers
gets updated only after we exit the Exercise
component because if we don't want userAnswers
changing which would change currentExercises
which could cause the exercise that gets shown in the Exercise
component to change in a way we don't want.
addExerciseSubmission({ | ||
variables: { exerciseId: exercise.id, userAnswer } | ||
variables: { exerciseId, userAnswer } |
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.
We no longer update the userAnswers
locally after each submit. Instead, we update the server exercise submissions and we update the localUserAnswers
because we don't want updating userAnswers
to change the currentExercises
which would cause a rerender in a buggy way when the exercise filtering checkbox is checked (e.g. if we only show incomplete exercises then we complete an exercise, then we would increment the exercise index and the exercises array would be shorter by 1 after that exercise got complete, so to avoid this we don't update userAnswers
until after the user exits the Exercise
component).
|
||
return ( | ||
<div className={`mx-auto ${styles.exercise__container}`}> | ||
<button | ||
className="btn ps-0 d-flex align-items-center" | ||
onClick={() => setExerciseIndex(-1)} | ||
onClick={() => onExit(localUserAnswers)} |
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.
We pass all the localUserAnswers
so that the parent component can update userAnswers
to reflect the current state.
onClick={() => { | ||
setExerciseIndex(i => i - 1) | ||
setAnswerShown(false) | ||
setMessage(Message.EMPTY) |
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 reason why we now have to set the message to EMPTY and set the answer to be not shown is because we are no longer completely rerendering the Exercise
component (it never gets completely rerendered now because we removed key
prop and because we don't update any parent state until after we exit the Exercise
component).
SOLVE EXERCISES | ||
</NewButton> | ||
</div> | ||
)} |
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 SOLVE EXERCISES button only shows if there are exercises left. So if the user completes all the exercises and then checks the exercise filtering checkbox, there will be no exercises left, so this button is not shown.
Maybe we can also add a message in the empty exercise list body like "Congratulations! You finished all the exercises!" and then maybe we can add a button that says "Go to the next lesson" or something similar.
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!
Changes:
How to test:
Desktop unchecked:
Desktop checked:
Mobile unchecked:
Mobile checked:
This pull request is related to #2253 and #2251