fix: Broken CSS styles of ora2 block editor [FC-0076] #36220
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
- It was this error 'Uncaught TypeError: el.timepicker is not a function' while rendering the editor.
It's fixed adding the timepicker pluging in `xblock_v2/xblock_iframe.html`
- Added '.openassessment_cancel_button' and '.openassessment_save_button' as action buttons.
DanielVZ96
force-pushed
the
chris/FAL-3992-fix-ora2
branch
from
6e4f191 to
bd4dee7
Compare
|
@ChrisChV I also added the open assessment styles so that it can display properly. I had to add a ton of css and maybe it can be reduced somehow. |
refactor: Use openassessment manifest.json to load css from dist
ChrisChV
changed the title
There was a problem hiding this comment.
Looks good 👍
- I tested this: I followed the testing instructions
- I read through the code and considered the security, stability and performance implications of the changes.
- I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
- Includes tests for bugfixes and/or features added.
- Includes documentation
- Includes fixtures that create objects required for manual testing.
There was a problem hiding this comment.
👍 Approved because this is working great! But I have questions about why, and whether we're including enough files from the ORA2 manifest.
- I tested this + fix: make it work in library v2 [FC-0076] edx-ora2#2272 (review)
- I read through the code
- I checked for accessibility issues
- Includes documentation -- could use some explanations in the ORA manifest xblock code.
-
User-facing strings are extracted for translationN/A
| openassessment_path = Path(openassessment.__path__[0]) | ||
| oa_manifest_path = openassessment_path / "xblock" / "static" / "dist" / "manifest.json" | ||
|
|
||
| if oa_manifest_path.exists(): | ||
| with open(oa_manifest_path, "r") as f: | ||
| oa_manifest = json.load(f) | ||
| new_oa_manifest = { | ||
| 'oa_ltr_css': oa_manifest.get("openassessment-ltr.css", ""), | ||
| 'oa_ltr_js': oa_manifest.get("openassessment-ltr.js", ""), | ||
| 'oa_editor_textarea_js': oa_manifest.get("openassessment-editor-textarea.js", ""), | ||
| 'oa_editor_tinymce_js': oa_manifest.get("openassessment-editor-tinymce.js", ""), | ||
| } | ||
|
|
There was a problem hiding this comment.
I've tested openedx/edx-ora2#2272 without this change and I see that it fixes the Authoring MFE's ORA2 editor experience. But I don't understand how it works -- don't find any code that uses these terms oa_manifest or oa_editor_textarea_js?
- Why is it ok to expose only the LTR resources (and not the RTL ones)? Don't we need both?
- Why don't we need the other resources in the manifest.json?
- Why don't we have to do this for other XBlocks?
Some comments would help :)
There was a problem hiding this comment.
don't find any code that uses these terms oa_manifest or oa_editor_textarea_js?
They are used in xblock_iframe.html (ref1, ref2)
Why is it ok to expose only the LTR resources (and not the RTL ones)? Don't we need both?
When we add the RTL style, it automatically applies that style (right-to-left reading) regardless of the language. I wasn't entirely sure how to place that conditional, so I think we should leave a comment so it can be done in a future task. @DanielVZ96 Could you add this comment?
Why don't we need the other resources in the manifest.json?
Two reasons: they are being renamed so they can be used in the template (the template does not accept scripts) and this files are only being used for the studio view (editor). @DanielVZ96 Could you add a comment for this?
Why don't we have to do this for other XBlocks?
The other Xblocks don't need extra files like these, ORA2 has a complicated UI that needed all these files to work.
There was a problem hiding this comment.
I added the comment @ChrisChV @pomegranited
ChrisChV
changed the title
github-project-automation
bot
moved this from Waiting on Author
to Done
in Contributions
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
* It was this error 'Uncaught TypeError: el.timepicker is not a function' while rendering the editor. It's fixed adding the timepicker pluging in xblock_v2/xblock_iframe.html * Added '.openassessment_cancel_button' and '.openassessment_save_button' as action buttons. * Use openassessment manifest.json to load css from dist
* It was this error 'Uncaught TypeError: el.timepicker is not a function' while rendering the editor. It's fixed adding the timepicker pluging in xblock_v2/xblock_iframe.html * Added '.openassessment_cancel_button' and '.openassessment_save_button' as action buttons. * Use openassessment manifest.json to load css from dist
Description
xblock_v2/xblock_iframe.htmlSupporting information
Testing instructions
npm run buildon cms-shellDeadline
ASAP
Other information
N/A