-
Notifications
You must be signed in to change notification settings - Fork 70
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
Conversation
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 |
👍
|
Looks good to me 👍
|
@sstack22 I think they need your input here. |
@catong FYI |
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 |
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. |
@pomegranited, I seem to have edit access! I'll make the requested changes and report here once done. |
@pomegranited, you may want to notify somebody about edx-documentation changes. I've whipped up a couple of suggested lines: |
@lamagnifica FYI for SOL-1805 |
Comments on the referenced commit, @arbrandes. I really appreciate you taking the time to update the doc as well. |
@lamagnifica updated doc changeset at arbrandes/edx-documentation@7e39731. @pomegranited as well, should a separate PR be issued for the above? |
@arbrandes Created and approved a documentation PR for you: PR #1218. |
Thanks, @pomegranited! |
@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.
@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. |
@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. |
@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. |
@bradenmacdonald Sweet, merging now. |
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
Latest commit
Testing instructions
staff@example.com
.F5
or equivalent) and scroll to the last line."event": {}
part of the line (it helps toCTRL-f
), and check that there is anitem
element with a value that matches the label of the item you manipulated above.Notes
Reviewers
Also tagging @pdesjardins as a docs FYI.