-
Notifications
You must be signed in to change notification settings - Fork 6
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
#6 and #7, Scoring and view other answers on score screen toggles, disabled by default #14
base: dev/1.1.0
Are you sure you want to change the base?
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.
The scoring mode works, which is a great start - but there's a lack of polish and a couple of bugs that we need to address:
UI/UX:
- "Next" button persists in the creator on the word selection page, but doesn't do anything.
- "Next" button in the creator should probably be highlighted or made more apparent due to its importance to the overall creator flow. The addition of extra buttons makes it stand out less visually.
- (This is a "while you're here" thing): the "Submit!" and "Reset" buttons in the player do not have cursor styles applied to them to properly indicate they are buttons, and word inputs could probably benefit from a modest background color to better highlight that they are in fact inputs.
Bugs:
- The "Reset" button in the creator resets the widget title but not the paragraph text (IMO, it shouldn't touch the title - just the paragraph).
- There is a major regression in loading/saving qsets from the master branch right now. If I had to guess, your branch may not have included the changes Golden made to address these. If I select "Let Me Choose" and pick a handful of words to hide, save, and then reload the creator, the word selection defaults back to "Automatic" and the prior selections were lost.
I left a couple in-line comments as well.
src/_score/score_module.php
Outdated
return 0; | ||
// if (isset($this->questions[$log->item_id])) return 100; | ||
if (isset($this->inst->qset->data['options']) && | ||
isset($this->inst->qset->data['options']['enableScoring']) ) |
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.
Individual conditions should be on a single line - or in a left-aligned block. Prefer this to be single-line.
@@ -36,6 +65,13 @@ protected function get_feedback($log, $answers) | |||
} | |||
|
|||
$final_words = []; | |||
//If options boolean is false it will show no feedback for other recorded responses |
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 put this down here, and not at the top of get_feedback
?
I have fixed all issues associated with the widget including the UI/UX ones. The next button has a distinct different color indicating its importance. It has changes from main branch that Golden fixed. I moved |
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.
To clarify, my comment above this line was associated with the condition below it. You added a condition to check whether feedback should be displayed (via the value of showAllOtherAnswersBoolean
), and short-circuit the get_feedback
method if it shouldn't. Why wouldn't we put that check at the top of get_feedback
? Everything else in that method is effectively ignored when showAllOtherAnswersBoolean
is false.
Your changes otherwise are solid and a big step forward, and everything works the way it should. Once you clean up get_feedback
and remove leftover trace statements and console logs, I can merge this.
This PR addresses issues #6 and #7 making toggle buttons in the creator for showing other answers in the scorescreen, as well as making the widget scorable. I’m closing my previous PR for #6 to consolidate everything into this one. For #7, I reused the toggle code and qset options logic from the earlier PR, integrating them to add scoring functionality when the toggle is enabled. These changes are compatible with older instances. I also fixed a bug from the previous PR where the toggle buttons wouldn’t sync properly during initFromExistingWidget.
There are still some outdated HTML4 tags and absolute styling in the old tooltips, which can cause the information to go off-screen. I plan to address this in a separate PR by replacing them with the new tooltip style I’ve created.
This update adds an option object to the qset with two new booleans:
showAllOtherAnswersBoolean
andenableScoring
, both of which are disabled by default.