-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: allow customizing passing grade #382
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
feat: allow customizing passing grade #382
Conversation
7f9fec8 to
15f1654
Compare
src/quiz/use-quiz.ts
Outdated
| validationMessages, | ||
| onSuccess, | ||
| onFailure, | ||
| passingGrade = 100, |
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 on the fence. I'm not sure if we should have a default value here, or just have it a required prop and ask clients to provide the value.
(#356 already contains a breaking change, so it's not a big deal if we have another 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.
I slightly prefer not defaulting, but either is fine. The main argument for not defaulting is that I don't think that's set in stone, so we may end up having to change it.
src/quiz/use-quiz.ts
Outdated
| const [questions, setQuestions] = | ||
| useState<Question<AnswerT>[]>(initialQuestions); | ||
| const [correctAnswerCount, setCorrectAnswerCount] = useState(0); | ||
| const [grade, setGrade] = useState(0); |
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.
@ojeytonwilliams I'd like to pick your brain about this 😄
Do you think the initial value of grade and correctAnswerCount should be null instead of 0? Being null would mean that the quiz hasn't been validated, and 0 would mean the quiz has been validated and the value is 0.
That's a little more work for us to handle typing, but client would then be able to conditionally render the grade and correctAnswerCount values using null check, rather than having to pair them with a hasSubmitted boolean.
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.
Good question! I think null (or perhaps undefined) makes sense, mostly because it doesn't seem right to say that someone has a grade or number of correct answers until they have been graded.
One thing to consider is a return type like this:
{ validated: true, grade: number, correctAnswerCount: number, ... } | { validated: false, grade: never, correctAnswerCount: never, ... }That way the client can, in principle, just check the value of validated, rather than having to account for both grade and correctAnswerCount being nullable. I say "in principle", because I'm not 100% sure how the code would look in practice, but I think it should work.
db0d59e to
9721d1e
Compare
src/quiz/use-quiz.ts
Outdated
| type ValidationData = | ||
| | { validated: true; grade: number; correctAnswerCount: number } | ||
| | { validated: false; grade: undefined; correctAnswerCount: 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 went with this because:
ojeytonwilliams
left a comment
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 got the type right this time 😰
Co-authored-by: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
ojeytonwilliams
left a comment
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.
LGTM 👍


Checklist:
Update index.md)Updates
useQuizto allow customizing the passing grade.Ref: https://github.com/freeCodeCamp/fCC10/issues/2#issuecomment-2391462203.