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

Added Question Bank Functionality #36

Merged
merged 11 commits into from
Apr 22, 2024

Conversation

dmols
Copy link
Contributor

@dmols dmols commented Dec 12, 2023

Users can now make widgets and select a few of the questions to be used as the question bank. The way the question bank is implemented, the set of total questions will be shuffled and then the widget will select as many to display as the user stated. I also added some guards so the set of questions wouldn't get shuffled twice (which could've happened if the user selects to randomize the qset). Please let me know if you find any errors or have any comments about the functionality/appearance of the changes; I know the CSS could likely use a boost.

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.

It's a good start, but I left a number of comments to address various issues. When it's functionally polished, we can work on adding styling.

@@ -202,12 +205,14 @@ Hangman.controller 'HangmanCreatorCtrl', ['$timeout', '$scope', '$sanitize', 'Re
$scope.attempts = ~~qset.options.attempts or 5
$scope.partial = qset.options.partial
$scope.random = qset.options.random
$scope.enableQuestionBank = qset.options.enableQuestionBank
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javascript/coffeescript/angular is letting you off easy here, but remember what I said about backwards compatibility: we can't always assume qsets will have these values, since every existing GTP qset saved prior to this feature won't have options.enableQuestionBank and options.questionBankVal. Instead of throwing a console error, it's just setting the $scope values to undefined. This is problematic later on when undefined values are saved instead of true or false.

@@ -202,12 +205,14 @@ Hangman.controller 'HangmanCreatorCtrl', ['$timeout', '$scope', '$sanitize', 'Re
$scope.attempts = ~~qset.options.attempts or 5
$scope.partial = qset.options.partial
$scope.random = qset.options.random
$scope.enableQuestionBank = qset.options.enableQuestionBank
$scope.questionBankVal = qset.options.questionBankVal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above line.

@@ -234,6 +239,13 @@ Hangman.controller 'HangmanCreatorCtrl', ['$timeout', '$scope', '$sanitize', 'Re
,
400

$scope.openQuestionBankDialog = ->
$scope.questionBankDialog = true
$scope.enableQuestionBank = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of setting enableQuestionBank to false here? The way you have this setup, even if I previously set enableQuestionBank to true, opening the dialog flips the value back to false.

$scope.questionBankDialog = true
$scope.enableQuestionBank = false

$scope.closeQuestionBankDialog = ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might try (if you haven't already) simply setting questionBankDialog = false as an expression in the associated ng-click value in the creator DOM, since creating a function to do that is redundant. Granted, I know angularjs can (sometimes) have issues with those expressions.

src/creator.html Outdated
<input type="radio" ng-model="enableQuestionBank" ng-value=false> No</input>
<input type="radio" ng-model="enableQuestionBank" ng-value=true> Yes</input>
</span>
<br/><br/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we can fix with a styling pass that's due anyhow, but as a general rule, avoid using <br />. It's an HTML element that serves no purpose beyond styling, which is the purview of CSS.

src/creator.html Outdated
<span ng-show="enableQuestionBank" >
<strong>How many questions to ask?</strong>
<br/>
<input class="numInput" type="number" ng-model="questionBankVal" ng-attr-min="{{items.length < 1 ? items.length : 1}}" ng-attr-max="{{items.length}}" step="1">
Copy link
Member

@clpetersonucf clpetersonucf Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ng-attr-min keeps the user from inputting an integer below 1 here, but the variable being bound to this input is questionBankVal, which is initially set to 0. You may want to set the default value of questionBankVal to 1 instead, so the input doesn't default to an illegal value. As a result, when I first open the dialog and enable the question bank, the input is blank.

@dmols
Copy link
Contributor Author

dmols commented Dec 15, 2023

@clpetersonucf Thank you for the feedback! I pushed changes that should fix all of the concerns, but please let me know if anything should be altered.

@dmols
Copy link
Contributor Author

dmols commented Jan 5, 2024

Note: It might be worth checking out the other PRs first (for Matching and Last Chance Cadet) since those are the ones I worked on after this one, and I added extra styling and logic to those

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.

A lot of my feedback here is in line with the comments I left for This or That, Last Chance Cadet, and Matching, including:

  • Use of semantic HTML
  • Reuse the existing modal BG, don't make a new one
  • Ensure the existing modal BG allows users to exit the question bank modal
  • Ensuring the user isn't trapped in the question bank modal prior to authoring any questions
  • CSS selector naming conventions and avoid use of words for colors

Additionally:

  • Make sure the question bank button in the header visually changes based on whether the feature is enabled or not
  • Add hover, focus styles to the question bank button
  • Make sure the question bank button has a cursor: pointer style to properly indicate its interactivity

@dmols
Copy link
Contributor Author

dmols commented Feb 13, 2024

Added all the requested changes to the code except for the focus on the question bank button, seeing as there seems to be default focus on the element already. Is there a specific kind of focus you'd like applied to the button? I could definitely add that.

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.

Same as This or That - these changes look complete, but we should make it clearer that enabling the question bank randomizes questions.

There's an easy and a harder way to do that - I think both are acceptable options:

  1. Add some text to the question bank dialog to indicate that when the feature is enabled, questions will be randomized, regardless of the state of the randomize toggle
  2. Have the toggle in the creator bound to BOTH the questionBank state and randomize option state, and force it into the ON position when the question bank is enabled

Once you implement one of those options, this ought to be good to go.

@dmols dmols requested a review from clpetersonucf April 1, 2024 14:58
@clpetersonucf clpetersonucf changed the base branch from master to dev/1.3.0 April 18, 2024 19:09
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.

All feedback addressed. Nice work.

@clpetersonucf clpetersonucf merged commit d140a1a into ucfopen:dev/1.3.0 Apr 22, 2024
1 check passed
@clpetersonucf clpetersonucf mentioned this pull request Apr 23, 2024
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