-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat: add experimental feature for direct XBlock rendering [FC-0035] #1281
feat: add experimental feature for direct XBlock rendering [FC-0035] #1281
Conversation
Thanks for the pull request, @Agrendalath! 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. |
826daf1
to
685fbe7
Compare
685fbe7
to
c032619
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1281 +/- ##
==========================================
+ Coverage 88.33% 88.38% +0.04%
==========================================
Files 291 292 +1
Lines 4929 4957 +28
Branches 1242 1249 +7
==========================================
+ Hits 4354 4381 +27
- Misses 561 562 +1
Partials 14 14 ☔ View full report in Codecov by Sentry. |
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.
@Agrendalath This is really interesting. I followed your instructions and tested this locally. The new way seems to be a bit slower as compared to current implementation, if I am not wrong this should improve when xblocks do not use global dependencies.
I also noticed some difference in style and missing styles and icons like below:
Download handout
button in videos is defaulting to current domain (MFE domain) instead of LMS, so it is not working.
This is probably a general issue where if you link any unit using the relative path (jump-id) for example: /jump_to_id/dacc88e550bd48db93899979bff1b086
, it defaults to current domain and fails. The current implementation goes to LMS domain by default.
Mathjax fails to render after submit when you have a simple number as input, for example:
Raises Uncaught SyntaxError: expected expression, got '<' formula_equation_preview.js:1
We probably need to write some tests or ignore the file for now to make sure we don't cause problems for other PRs (coverage failure).
Eventually - yes. Once we implement the support for The problems you noted seem to be happening in the original version (the one in the |
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.
@Agrendalath Got it. Looks good to me.
👍
- I tested this: (played around with xblocks locally)
- I read through the code
-
I checked for accessibility issues -
Includes documentation
@brian-smith-tcril, would you be able to review this as the CC? |
I think @ormsbee will want to take a look at the whole thing as part of FC-0035 - particularly the backend PR - first. I can come back here once the concept is approved. |
OSPR management note: As of today this PR is still blocked on openedx/edx-platform#34161 (ref). |
Closing for now, as mentioned in openedx/edx-platform#34161 (comment). |
@Agrendalath 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 ports the XBlock iframe wrapper, originally designed for the Library Authoring MFE. Instead of embedding the LMS view in an iframe, this approach queries XBlock v2 API to get the fragment's HTML and resources, then it assembles a tag with all necessary assets and then finally puts ready HTML markup in an iframe.
This is an experimental approach that will need lots of improvements to reach the feature parity with the old view. Some known bugs/missing features:
.inline-discussion-thread-container
is not rendered in Discussion XBlocks. As they had been replaced by the Discussions sidebar, fixing this might not be necessary.openedx-learning
, fixing this might not be necessary.Supporting information
Required backend changes: openedx/edx-platform#34161.
Testing instructions
RENDER_XBLOCKS_EXPERIMENTAL=true
infrontend-app-learning/.env.private
.RENDER_XBLOCKS_DEFAULT=false
infrontend-app-learning/.env.private
.Deadline
"None"
Other information