Skip to content
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

Open
wants to merge 14 commits into
base: dev/1.1.0
Choose a base branch
from

Conversation

chrissolanilla
Copy link

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 and enableScoring, both of which are disabled by default.

Copy link
Member

@clpetersonucf clpetersonucf left a 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.

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']) )
Copy link
Member

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
Copy link
Member

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?

@chrissolanilla chrissolanilla marked this pull request as draft January 31, 2025 22:14
@chrissolanilla chrissolanilla marked this pull request as ready for review February 3, 2025 18:55
@chrissolanilla
Copy link
Author

chrissolanilla commented Feb 3, 2025

Screenshot 2025-02-03 at 1 57 14 PM

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 $final_words = []; to the top of the function, as well as making individual conditions on a single line. It also fixes the tooltip on the title that used to go off screen and now additionally the buttons in the player have a cursor style of pointer indicating a button.

Copy link
Member

@clpetersonucf clpetersonucf left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants