Skip to content

Conversation

abodacs
Copy link
Contributor

@abodacs abodacs commented Sep 24, 2022

Description

This PR adds HTML support in explanations within DnDv2

After:
screenshot-rendered-html

Supporting information
OpenCraft internal ticket : BB-6630

Testing instructions

  1. Checkout this branch in <devstack_parent>/src directory in your local setup
  2. log in to Studio shell, switch to edxapp virtual env
  3. Go to /edx/src/xblock-drag-and-drop-v2 and run pip install -e .
  4. enable the Drag and Drop XBlock in Studio
  5. Go to Studio UI and create a new unit
  6. Select Problem > Advanced > Drag and Drop, to create a v2 drag and drop XBlock
  7. Click Edit and change mode to Assessment
  8. Scroll down to Explanation Text like Test HTML 12m<sup>2</sup>
  9. Add some text in the text area
  10. Click Continue twice and Save
  11. Click on View Live Version
  12. Ensure there is no extra line as shown above
  13. Do as many attempts as required till "Show Answer" button is active
  14. Click on Show Answer
  15. Ensure Explanation is rendered like 'Test HTML 12m2'

Deadline

"None"

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 24, 2022
@openedx-webhooks
Copy link

openedx-webhooks commented Sep 24, 2022

Thanks for the pull request, @abodacs! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks
Copy link

Thanks for the pull request, @abodacs! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

⚠️ We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

@natabene
Copy link

@abodacs Thanks for the PR, we will be waiting for the CLA check to go green first.

Copy link
Contributor

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

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

@abodacs Looks close to being done. Thank you for your patience. I have added a couple of suggestions. I will add my approval once those are addressed.

Copy link
Contributor

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

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

@abodacs 👍

  • I tested this: Entered HTML in the explanation box and verified that it renders properly in the window
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation - NA

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

This one looks mostly good. I left few small comments.

@Agrendalath
Copy link
Member

@natabene, would you like to take a look at this before we merge it?

@natabene
Copy link

@Agrendalath I personally don't, but maybe @jmbowman wants.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: checked that explanations render HTML correctly; verified Maple and Master compatibility
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

@e0d
Copy link
Contributor

e0d commented Oct 13, 2022

Did this change consider implications for cross-site scripting (XSS)? If we're adding HTML support we should ensure that we're not creating an XSS issues. There are some useful docs here.

@Agrendalath Agrendalath merged commit b801d98 into openedx:master Oct 13, 2022
@openedx-webhooks
Copy link

@abodacs 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@openedx-webhooks
Copy link

@abodacs 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants