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

fix: fix questions formatting issues #208

Conversation

Cup0fCoffee
Copy link
Contributor

@Cup0fCoffee Cup0fCoffee commented Jan 22, 2023

Description

This PR fixes issues demonstrated in #172.

There are issues with how the problem's questions are displayed if some formatting is applied to them. The issues are:

  1. OLXParser doesn't recognize style attributes when building HTML from JSON
  2. Both OLXParser and ReactStateOLXParser don't respect the order of the question layout when parsing and building

Steps to replicate and tests

Prerequisites

Setup local devstack and use README to setup this repo for local development.

NOTE: main version of this repo wasn't working for me at the time of writing, version v1.73.0 was used.

Instructions

  1. In any course create a new problem
  2. Add some text to the problem's question
  3. Add some styling to the question's text, e.g. make some text bold or italic, change text color, etc.

When doing it without these changes, after saving, both in studio and course-authoring MFE the question's text will appear out of order (the styled text will come first, followed by unstyled text), and in course-authoring MFE, the style attributes will be rendered to tags instead of attributes.

When testing with these changes, you should create a new problem block, because once the changes are saved, the out of order structure will be saved to the DB, so it will still appear broken even with changes.

Important notes for reviewers

This PR is based on v1.73.0, because main wasn't working fine. Once the changes are approved, I can rebase the branch onto main for merging.

Private-ref: BB-7021

@openedx-webhooks
Copy link

Thanks for the pull request, @Cup0fCoffee! 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 openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 22, 2023
@Cup0fCoffee Cup0fCoffee changed the title fix: fix question formatting issues fix: fix questions formatting issues Jan 22, 2023
Copy link
Contributor

@kaustavb12 kaustavb12 left a comment

Choose a reason for hiding this comment

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

👍

Great job here, the changes work as advertised.
I have added a few minor comments. Please take a look.

  • I tested this: Tested the scenario reported in the issue
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

Copy link
Member

@farhaanbukhsh farhaanbukhsh left a comment

Choose a reason for hiding this comment

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

Thank you for such amazing work, Maxim and Kaustav. I have added few nits.

I have also noticed few lint issues so please run npm run lint to see those issues and ./node_modules/.bin/fedx-scripts eslint --ext .js --ext .jsx --fix . to fix them.

@Cup0fCoffee Cup0fCoffee force-pushed the maxim/fix-question-formatting-issues branch from f6d7042 to f9fbebe Compare February 1, 2023 15:43
openedx#173

Fixed the issues of the problem editor related to displaying, editing
and saving of the problem questions which were styled:
* Fix the order in which the styled text was being displayed - now
  displayed in the correct order, instead of putting the styled text
  first
* Render the style attributes of the HTML tags correctly, instead of
  rendering them as HTML tags
@Cup0fCoffee Cup0fCoffee force-pushed the maxim/fix-question-formatting-issues branch from f9fbebe to 4ade3c9 Compare February 1, 2023 15:46
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 7, 2023
@KristinAoki
Copy link
Member

KristinAoki commented Feb 7, 2023

Thank you so much for the work! The first issue (style tags) has recently been resolved by PR 229. The second issue regarding the preservation of the question is currently being reviewed in PR 232. @Cup0fCoffee, do you want to review these PRs and see that everything from this PR is covered in them?

@openedx-webhooks
Copy link

@Cup0fCoffee Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@Cup0fCoffee Cup0fCoffee deleted the maxim/fix-question-formatting-issues branch February 25, 2023 15:32
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 27, 2023
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.

7 participants