-
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
Implemented question bank functionality #65
Implemented question bank functionality #65
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.
Creator:
- UI additions look OK, but styling needs to match the rest of the creator UI. One of us can help with that.
- The UI should indicate whether question banks are enabled without having to enter the question bank dialog (make it part of the button text?)
- The question bank values are never actually saved to the QSet.
- It'd be nice if when the question bank toggle is initially flipped to enabled, the default question bank size matches the total item count, and doesn't just start at 1. Obviously we don't want the question bank value to be overwritten when the total item count changes, so it should only update to the total item count if the question bank isn't enabled.
Player:
- Code additions look OK, but can't yet test the actual question bank behavior. Please remember to actually test the functionality of your code!
- Backwards compatibility is provided, which is good.
Fixed most of the issues addressed in the review, minus the UI style changes. I'm thinking you mean the QB button needs to match the 'randomizeOrder' button, but wasn't sure. Happy to tag-team the CSS changes needed to make this visually ready for merge. |
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.
Functionally, this is much better. Good improvements. Most of my new feedback is related to styling and your actual implementation of various elements.
In addition to the comments below, consider:
The question bank modal in the creator can follow the visual language already present in the other modal dialog already present in the creator: the tutorial dialog. That means:
- No border, and a wider
border-radius
. - Thinner
font-weight
(the tutorial is using300
("light") instead of400
, the default.) - Confirm button should be green, since that's the color established for buttons that perform actions. Make sure it's the same color value.
…to watch questionBankVal
All the requested changes have been made. Only issue I find is that the value in |
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 is very, very close to being ready. Nice job incorporating all the feedback I've given you 👍
The only thing I'm hung up on: enabling the question bank randomizes the questions, but that's not communicated anywhere in the creator. The question bank dialog should probably include a message to indicate that questions will be randomized.
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.
All requested changes have been made. This looks good, and works great. I might do some small styling adjustments, but there's nothing that should stop this PR from being approved.
Question bank is now fully implemented for this widget. I added a good amount of styling to the dialog but please let me know if you have any suggestions to make it fit the theme of the widget a little better, or any in general to make the question bank function better!