-
Notifications
You must be signed in to change notification settings - Fork 65
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
Prevent score point deduction #97
base: master
Are you sure you want to change the base?
Prevent score point deduction #97
Conversation
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 am only hinting to technical issues here, see comments.
The issue with users being enabled to simply mark all the words without thinking and receive full score automatically should be addressed by someone from the H5P core team.
scripts/mark-the-words.js
Outdated
@@ -472,7 +473,7 @@ H5P.MarkTheWords = (function ($, Question, Word, KeyboardNav, XapiGenerator) { | |||
if (word.isCorrect()) { | |||
result.correct++; | |||
} | |||
else if (word.isWrong()) { | |||
else if (word.isWrong()&&!self.params.behaviour.preventPointSubstraction) { |
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.
It's required to add blanks around &&
to follow the H5P coding style guidelines.
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.
Thank you for this hint.
library.json
Outdated
@@ -3,7 +3,7 @@ | |||
"description": "Test your users by making them select the correct words from a text.", | |||
"majorVersion": 1, | |||
"minorVersion": 11, | |||
"patchVersion": 0, | |||
"patchVersion": 1, |
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, bumping the patch version is not strictly required by the change alone (yes, if you want to deploy it, but not on a development system to test), so version number management should be left to the release manager.
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.
Ok, I switched back to patchVersion 0
@@ -208,6 +208,14 @@ | |||
"description": "Show points earned for each answer.", | |||
"importance": "low", | |||
"default": true | |||
}, |
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.
If you add fields to semantics.json
, you'll need to update all the translation files, not just the German one. Otherwise, content in other languages may break. If you cannot translate the strings yourself, you need to use the English text.
The H5P CLI tool can help you with updating translations. You can mark fields with one of the appropriate attributes "status": "new"
, "status": "updated"
or "status": "removed"
. If you then run h5p update-translations <the-repository-name>
, the CLI tool will update all the translations as required and use the English texts as default. One would translate them for real afterwards.
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, I knew that I shouldn't only update the German language file but didn't know how to change all language files. Thank you so much for clearifying this. As for h5p update-translations <the-repository-name>
I received the error "unknown command", but I managed it with the h5p help
that suggested add-english-texts <language-code> <library>
. Maybe I have to update my H5P CLI tool - the update-translations order would be much more comfortable...
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.
@osa-freiburg Nope, the command is update-translations
. The command add-english-texts
does something else (that I am not even sure what for) and lead to #97 (review)
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.
Ok, thank you. I have to check this, but probably not today any more, but the day after tomorrow. I'll come back...
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.
As it was not a big thing, I just fixed the faulty translation files. The error message was generated from an old h5p CLI - sorry, that I didn't update it in the beginning. Now the update-translations
was no problem and all the "englishLabel" and "englishDefault" values disappeared. So the language files should be ok now. Thanks for reviewing!
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.
Ah, one thing FYI: I had to remove the "status": "new"
manually in the semantics.json.
semantics.json
Outdated
"importance": "low", | ||
"description": "Enable to prevent score point deduction for wrong marked words (be careful: might be abused from users who simply mark all the words to get full score)", | ||
"default": false, | ||
"status": "new" |
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.
Hmm, I don't think this went as it should have. "status": "new"
should be gone automatically once H5P CLI has finished and and all the "englishLabel"
and "englishDefault"
values inside the language files should not be there.
Add a parameter to the behavior options to prevent deduction of score points when user is marking wrong words.