-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix: fix questions formatting issues #208
Conversation
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:
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. |
There was a problem hiding this 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.
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
src/editors/containers/ProblemEditor/data/ReactStateOLXParser.js
Outdated
Show resolved
Hide resolved
f6d7042
to
f9fbebe
Compare
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
f9fbebe
to
4ade3c9
Compare
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? |
@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. |
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:
OLXParser
doesn't recognize style attributes when building HTML from JSONOLXParser
andReactStateOLXParser
don't respect the order of the question layout when parsing and buildingSteps 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, versionv1.73.0
was used.Instructions
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
, becausemain
wasn't working fine. Once the changes are approved, I can rebase the branch ontomain
for merging.Private-ref
: BB-7021