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

[SOL-1998] Implement Show Answer button #101

Merged
merged 5 commits into from
Sep 22, 2016
Merged

Conversation

itsjeyd
Copy link
Contributor

@itsjeyd itsjeyd commented Sep 12, 2016

Description

In assessment mode, when the learner has run out of attempts, enable a "Show Answer" button. Clicking it causes all items to move to a correct placement.

Author

@arbrandes

OpenCraft review via open-craft#10.

Dependencies

None

JIRA

Sandbox URLs

Latest commit

Testing instructions

Assessment mode

  1. Go to the assessment mode DnDv2 instance on the LMS, and log in as staff@example.com.
  2. Click on Staff Debug Info, Delete Student State, and refresh the page.
  3. Notice that there is a "Show Answer" button, because this is in assessment mode.
  4. Notice that the "Show Answer" button is disabled, because this is your first attempt.
  5. Pick any item and drop it onto any zone.
  6. Click submit, which will make you run out of attempts (there's only one).
  7. Notice that the "Show Answer" button is now enabled.
  8. Click the "Show Answer" button. Observe that:
    • All items are placed correctly
    • Decoy items are still in the bank.
    • You can't drag any of the items.
    • The "Show Answer" button is disabled.
  9. Refresh the page, and see that your final attempt is displayed. Note: This currently only works if you did not misplace any items; a decoy item that was placed on the board counts as a misplaced item. See "Notes" below for more info.
  10. Click "Show Answer" again. The same thing should happen again.

Standard mode

  1. Go to the standard mode DnDv2 instance on the LMS, and log in as staff@example.com.
  2. Notice that there is no "Show Answer" button, because this is in standard mode.

Notes

There is an existing issue (present on master) that causes misplaced items to return to the item bank when refreshing the page after submitting the last remaining attempt for a DnDv2 problem. This PR does not include a fix for this issue. As a result, refreshing the page as described in step 9 above only produces expected results if items that were placed on the board in the final attempt are located in correct zones.

Reviewers

Also tagging @pdesjardins as a docs FYI.

@openedx-webhooks
Copy link

Thanks for the pull request, @itsjeyd! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx-solutions%2Fxblock-drag-and-drop-v2&number=101

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 15, 2016

@arbrandes This is looking great! 👍

  • I tested this: Feature works as advertised in PR description.
  • I read through the code
  • Includes documentation

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 15, 2016

@cahrens @staubina This is ready for you to have a look. Sandbox links are in the PR description. Thanks!

@staubina
Copy link
Contributor

staubina commented Sep 15, 2016

  • Tested according to test steps
  • Tested around the steps listed

I am reviewing the code changes still, but functionally it looks good.

Copy link
Contributor

@staubina staubina left a comment

Choose a reason for hiding this comment

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

@sstack22 @cahrens
Is there any concern with the functionality of this Show answer button vs. what we currently have incoming with the Capa updates? They seem to work differently and are enabled/disabled at different times.

@cahrens
Copy link

cahrens commented Sep 15, 2016

I think we should defer to product, and I'd recommend holding off on further review until product signs off. FYI, @sstack

Things that jumped out to me-- Hide Answer, icon. Not sure what else is different.

@staubina
Copy link
Contributor

I have a question posted that is related to other ongoing work with Capa and that is all right now.

@cahrens Can you take a look at this as well when you have a moment? I currently do not see anything that is concerning.

I am going to hold off pending @sstack22 comments and review.

@cahrens
Copy link

cahrens commented Sep 15, 2016

I will wait to look over the PR until product weighs in.

@sstack22
Copy link

@arbrandes - a couple comments:

@arbrandes
Copy link
Contributor

Hi @sstack22, I've addressed your comments on adcbf1c (@itsjeyd, you may want to update the PR description). The sandbox should be updated by the time you read this.

Regarding the box highlight when pressing Show Answer: that is indicative of moving the keyboard focus to the first drop zone. For now, I removed that altogether. Nevertheless, it seems from TNL-4936 that when clicking Show Answer, the keyboard focus should be moved to the problem "meta" area. The closest thing in the DnDv2 XBlock is the item bank. If we move the focus there, however, it will be highlighted similarly: is that ok? Or should the keyboard focus not be moved at all?

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 19, 2016

Thanks @arbrandes, description updated.

@staubina
Copy link
Contributor

When I answer a problem incorrectly and they click show answer the incorrect answer feedback popup persists, is this intentional?

screen shot 2016-09-19 at 11 11 02 am

@staubina
Copy link
Contributor

One comment on the feedback noted while testing in the Assessment mode.

Other than that Looks good to me. 👍

@cahrens
Copy link

cahrens commented Sep 19, 2016

Where is keyboard focus going on "Show Answer"?

@cahrens
Copy link

cahrens commented Sep 19, 2016

Except for my question about "Show Answer" (and AJ's question above), 👍

@arbrandes
Copy link
Contributor

@staubina, thanks for the review! Regarding the persistence of feedback when clicking Show Answer, as far as I know it should not happen. Plus, it's a trivial fix, so I'm including it in my next push.

@cahrens, thanks as well. The keyboard focus currently doesn't leave the Show Answer button after pressing it. I asked above whether I should move it to the item bank. What do you think?

@cahrens
Copy link

cahrens commented Sep 20, 2016

@arbrandes to be consistent with how capa problems will work, the focus should move to the top of the problem so that the learner can easily navigate through and "see" the answer.

In assessment mode, when the learner has run out of attempts, enable a
"Show Answer" button.  Clicking it causes all items to move to a correct
placement.
On the final attempt, misplaced items are not returned to the bank.
Change the "misplaced" feedback message to reflect this.
pylint correctly surmised that `test_grade` is an event test, so move it
over.
Fix padding on the reset button so that all separators on the sidebar
end up with the same height, and add padding to the top of the sidebar
container to distance the separators from the top edge.
@arbrandes
Copy link
Contributor

@cahrens, roger that! In this latest push (4c6fb7d), the focus moves to the item bank, the top-most element that can be focused on. The sandbox has been updated.

@cahrens
Copy link

cahrens commented Sep 22, 2016

👍

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 22, 2016

Thanks @cahrens and @staubina! This looks like it's good to go.

@sstack22 Can I get you to give this a thumbs-up as well? Thanks!

@sstack22
Copy link

@itsjeyd 👍 on my end!

@itsjeyd
Copy link
Contributor Author

itsjeyd commented Sep 22, 2016

Thanks @sstack22, merging now.

@itsjeyd itsjeyd merged commit 85c6143 into openedx:master Sep 22, 2016
@arbrandes arbrandes deleted the SOL-1998 branch September 27, 2016 21:47
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.

6 participants