-
Notifications
You must be signed in to change notification settings - Fork 14
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
color coding for answers #500
Conversation
This reverts commit 44094ff.
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 still see major problems with the code. Please make sure that you are submitting the final code for review.
assets/js/assessment_v2.js
Outdated
} | ||
|
||
populateQuestions(); | ||
addEventListener_explanations(); | ||
addEventListener_checkbox(); | ||
submitButton.addEventListener("click", showResults); | ||
|
||
populateQuestions(); |
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.
Why have calls to these functions been repeated?
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, it was a mistake I might have made while merging.
For the issue - #499
For fixing issues : virtual-labs/svc-bug-report#12 #506
This fix is with reference to the issue : Form submission and Include screenshot - functionality needs fixing #13 (link - virtual-labs/svc-bug-report#13)
assets/js/event-handler.js
Outdated
@@ -1,8 +1,11 @@ | |||
"use-strict"; | |||
|
|||
// Ensure dataLayer is defined Globally before using it | |||
window.dataLayer = window.dataLayer || []; |
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.
dataLayer has not been used in the changed code anywhere. Why are we re-initializing it? This may affect other code which may be doing similar checks.
If you want to avoid errors which may arise due to pushing into an undefined dataLayer object, you may add a conditionalto check if dataLayer is defined before you push into it
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 code was modified to fix the error mentioned in the issue #506
I agree with you about adding a conditional check before pushing, and will modify the code accordingly. Thanks.
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.
Raj, I tried to re-create the error by removing the global setting commit, but somehow I am not able to reproduce the bug. Now, if I make changes with just conditional checks instead of global settings, I will not be able to test it. Kindly advice if I should go ahead with these changes, then I will commit accordingly.
|
||
// if answer is correct | ||
if (userAnswer === currentQuestion.correctAnswer) { | ||
// Color the correct answer lightgreen |
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 code that was marking all options in black has been removed. What happens when the user submits a quiz once, then goes back, changes some of the options and submits it again.
In this case some of the wrong options would be marked red and others would be marked black.
Please test this scenario before we can move ahead with this PR.
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.
In this case some of the wrong options would be marked red and others would be marked black.>>> yes, it does exactly the same. That way the user understands the number of wrong attempts made. If the user reaches the right answer, that will also made green. I have followed up the previous green-red coloring logic. I am not sure why we will make the other options black. It starts with black, and according to selection, the text color changes. If we have a restart quiz button, we can make all the answers black again. Please let me know if my understanding of the fix itself is wrong.
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 kind of agree with your argument. Let's run it through Priya once to see what she thinks about it.
Fixed the issue - Color-coding of the answers for pretest and posttest pages needs to be fixed. #499 (#499)