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

Dev/1.2.0 #70

Merged
merged 37 commits into from
Jun 24, 2024
Merged

Dev/1.2.0 #70

merged 37 commits into from
Jun 24, 2024

Conversation

clpetersonucf
Copy link
Member

@clpetersonucf clpetersonucf commented Apr 15, 2024

dmols and others added 30 commits January 11, 2024 13:26
Implemented question bank functionality
…update-method

Adds missing update method to score screen
Copy link
Contributor

@dmols dmols left a comment

Choose a reason for hiding this comment

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

Most of the changes work well, though I did find a few issues in the creator and player side of the widget.

Here are a few:

  • Pressing the 'Tab' button in the player allows the user to move straight to the "Next" button even if an answer isn't selected. This makes the background visible to the user like so:

Screenshot 2024-04-30 at 3 27 10 PM

  • Related to the previous issue, skipping to the next question without an answer selected takes it out of the question total pool, when the score screen is loaded. The score screen actually just considers the questions the user selected an answer for, so technically as long as the user selects one correct answer and skips the rest, they'll get 100% on the widget.

  • 'Embed video' button isn't set as a requirement to be pressed, so the widget creator can fill out the remaining components and end up with this in the player.

Screenshot 2024-04-30 at 3 56 08 PM

  • Using the 'Q' button to hear the question only works when VO is on. Since the other keys work without VO on, it might be worth specifying that in the keyboard controls modal, if that's how it was originally intended to work.

@dmols
Copy link
Contributor

dmols commented Apr 30, 2024

Update: The first two issues I listed seem to only appear in older versions of Google Chrome (I was on 101.0.4). Something about old versions of browsers, that makes the navigation all wonky. On the latest version of Chrome now, and I'm only encountering the last two issues.

@clpetersonucf clpetersonucf requested a review from dmols June 11, 2024 19:08
@clpetersonucf
Copy link
Member Author

Update: The first two issues I listed seem to only appear in older versions of Google Chrome (I was on 101.0.4). Something about old versions of browsers, that makes the navigation all wonky. On the latest version of Chrome now, and I'm only encountering the last two issues.

The first of these two is kinda by design, you shouldn't be able to skip questions but if you do, then a log for that question is not generated. The score module will only process questions for logs it receives. Assuming the first issue is resolved with a browser update, the second issue shouldn't occur.

Made a small addition to address the second bug by validating the video URL on blur events as well as by clicking on the Embed Video button. Ready for re-review @dmols

Copy link
Member

@dgwn dgwn left a comment

Choose a reason for hiding this comment

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

All three noted features/fixes have been tested:

  • Question bank feature is functional; properly randomizes questions, randomization toggle is disabled when QB is enabled
  • Score screen changes allow play feedback to update properly. When switching between play attempts, the selected answers for that play are immediately reflected
  • Optional feedback is no longer required during the widget tutorial\ (a welcome change).

LGTM 👍

Copy link
Contributor

@FrenjaminBanklin FrenjaminBanklin left a comment

Choose a reason for hiding this comment

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

Can't see any issues. Does what it says on the tin.

Copy link
Contributor

@dmols dmols left a comment

Choose a reason for hiding this comment

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

Fixes look good! I only noticed a few things that could potentially be handled better:

  • The div that shows the questions remaining shows "of <total_questions>" first, rather than the actual number for the question you're at. You get to see this once you select an answer or hit the instructions button, for example. I feel it should probably list the number of the question first, then pan to the number of questions total.
  • There is no confirmation dialog before completely deleting a question in the creator side. Not sure of the easiest way to go about addressing this, since there isn't much space for say, a "Delete Question?" somewhere near the X. The 'X' button being right next to the title text -- at least with the button in its current size -- could make the user think they're clearing the title, rather than removing the whole question card.

Might just be me nitpicking, so feel free to disregard this and mark the PR as approved, as the big issues have already been addressed and resolved.

@dmols
Copy link
Contributor

dmols commented Jun 21, 2024

My suggestions above aren't related to the fixes made on this PR, so I'll add them as potential fixes on the issue board, once it's merged in. Changes here look good and widget runs great!

@clpetersonucf clpetersonucf merged commit 6e76d4f into master Jun 24, 2024
2 checks passed
@clpetersonucf clpetersonucf deleted the dev/1.2.0 branch June 24, 2024 17:32
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.

4 participants