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-1805] Add "item" field to item.dropped event #97

Merged
merged 1 commit into from
Sep 7, 2016

Conversation

pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Aug 25, 2016

Description

Adds a new "item" field to the edx.drag_and_drop_v2.item.dropped event data. It contains the item text label when present, otherwise the item imageURL. This makes it consistent with the location field, which contains the zone text label.

Dependencies

None

JIRA

Sandbox URLs

  • LMS: a predeployed DnDv2 instance
  • Event log: a constantly updated event log, filtered for the item.dropped event.

Latest commit

Testing instructions

  1. Go to the pre-deployed DnDv2 instance on the LMS, and log in as staff@example.com.
  2. Pick any item and drop it onto any zone.
  3. Open the event log URL, refresh it (with F5 or equivalent) and scroll to the last line.
  4. Look for the "event": {} part of the line (it helps to CTRL-f), and check that there is an item element with a value that matches the label of the item you manipulated above.

Notes

  1. This change only affects tracking events, and documentation, nothing visible to learners or instructors. So accessibility and internationalization should be unaffected.

Reviewers

Also tagging @pdesjardins as a docs FYI.

@openedx-webhooks
Copy link

Thanks for the pull request, @pomegranited! 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=97

@pomegranited
Copy link
Contributor Author

👍

  • I tested this by installing this PR's branch on my devstack, publishing a DnDv2 unit in my test course, and watching the tracking logs while I interacted with it. Tested with text and image items.
  • I read through the code.
  • Includes documentation for the added item tracking info, and the additional location documentation as specified by SOL-1805.

@staubina
Copy link
Contributor

staubina commented Aug 25, 2016

Looks good to me 👍

  • Tested on Sandbox
  • Review code for issues

@staubina
Copy link
Contributor

@sstack22 I think they need your input here.

@pomegranited
Copy link
Contributor Author

Thanks, @staubina ! @sstack22, is there anything you need fixed?

@cahrens
Copy link

cahrens commented Aug 29, 2016

@catong FYI

@sstack22
Copy link

Hi @pomegranited - I just discussed with our Analytics team. Would you be able to update the DnDv2 event design wiki with an example? https://openedx.atlassian.net/wiki/display/AN/DnDv2+XBlock+Event+Design

CC: @stroilova

@pomegranited
Copy link
Contributor Author

ping @arbrandes Do you have access to edit https://openedx.atlassian.net/wiki/display/AN/DnDv2+XBlock+Event+Design as requested above?

If not, let me know and I can do it.

@arbrandes
Copy link
Contributor

@pomegranited, I seem to have edit access! I'll make the requested changes and report here once done.

@arbrandes
Copy link
Contributor

@pomegranited, you may want to notify somebody about edx-documentation changes. I've whipped up a couple of suggested lines:

arbrandes/edx-documentation@8c1bf79

@catong
Copy link

catong commented Aug 29, 2016

@lamagnifica FYI for SOL-1805

@lamagnifica
Copy link

Comments on the referenced commit, @arbrandes. I really appreciate you taking the time to update the doc as well.

@arbrandes
Copy link
Contributor

@lamagnifica updated doc changeset at arbrandes/edx-documentation@7e39731.

@pomegranited as well, should a separate PR be issued for the above?

@pomegranited
Copy link
Contributor Author

@arbrandes Created and approved a documentation PR for you: PR #1218.

@arbrandes
Copy link
Contributor

Thanks, @pomegranited!

@bradenmacdonald
Copy link
Contributor

@arbrandes This needs a rebase.

What is left until we merge this? +1 from @sstack22 ?

Adds a new "item" field to the edx.drag_and_drop_v2.item.dropped event
data. It contains the item text label when present, otherwise the item
imageURL.

This makes it consistent with the location field, which contains the
zone text label.
@arbrandes
Copy link
Contributor

@bradenmacdonald, rebased and sandbox updated.

@pomegranited, you may want to update the PR description with the new HEAD, c78dbc6.

Regarding the test failure: I can't reproduce it locally. Plus, it worked on open-craft#9, so I'm guessing it's a Travis quirk.

@pomegranited
Copy link
Contributor Author

@arbrandes I've updated the description, thank you!

@bradenmacdonald Yep, I think we're just waiting for @sstack22 's ok, since this change doesn't affect accessibility.

@bradenmacdonald
Copy link
Contributor

@pomegranited Since @sstack22 is on vacation this week, has already looked at this, and you've already addressed the request to "update the DnDv2 event design wiki", I think this is good to merge.

@pomegranited
Copy link
Contributor Author

@bradenmacdonald Sweet, merging now.

@pomegranited pomegranited merged commit d6b3696 into openedx:master Sep 7, 2016
@arbrandes arbrandes deleted the SOL-1805 branch September 27, 2016 21:48
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.

9 participants