feat: support rendering XBlocks in the Learning MFE [FC-0035] - #34161
feat: support rendering XBlocks in the Learning MFE [FC-0035]#34161Agrendalath wants to merge 1 commit into
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. |
Rendering blocks directly in the MFE was developed for the Library Authoring MFE. The solution was then ported to the Learning MFE. This commit includes backend changes required to retrieve data of non-blockstore XBlocks from LMS.
37093fb to
b13c287
Compare
navinkarkera
left a comment
There was a problem hiding this comment.
- I tested this: Followed test instructions from openedx/frontend-app-learning#1281 and tested different types of xblocks locally.
- I read through the code
-
I checked for accessibility issues -
Includes documentation
|
Hi @ormsbee and @arbrandes! Just checking in on this. I think we're waiting on this to close the project. |
ormsbee
left a comment
There was a problem hiding this comment.
My apologies, but I think there's something fundamental about how this is supposed to work that I'm not understanding–particularly around why we're doing branching based on isinstance checks against BlockUsageLocator, and why it's okay to change the contract for student_view_data.
| // assumes courseware.html's loaded this method. | ||
| $(function () { | ||
| setup_debug('${element_id | n, js_escaped_string}', | ||
| (typeof setup_debug !== 'undefined') && setup_debug('${element_id | n, js_escaped_string}', |
There was a problem hiding this comment.
Please add a comment about why this change is necessary.
| # The second method is more efficient, but it doesn't work with the DescriptorSystem. | ||
| display_name = block.display_name | ||
| else: | ||
| display_name = get_block_display_name(block) |
There was a problem hiding this comment.
Doesn't get_block_display_name just check for the display_name attribute with fallback? Why does it break?
There was a problem hiding this comment.
Also, when would usage_id not be a BlockUsageLocator?
| if "student_view_data" in includes: | ||
| data["student_view_data"] = block.student_view_data() if hasattr(block, 'student_view_data') else None | ||
| if isinstance(usage_id, BlockUsageLocator): | ||
| data["student_view_data"] = render_block_view(block, 'student_view', None).content |
There was a problem hiding this comment.
Isn't the content the HTML rendering? Shouldn't student_view_data always be a JSON serializable dict on the XBlocks that support it?
|
Hi @Agrendalath and @ormsbee! Just checking in on this. |
|
We've found that the Library Authoring approach to using an iframe per XBlock doesn't give users a good enough UX. See my comment in the PR that was meant to add the same approach to Course Authoring: openedx/frontend-app-authoring#964 (review) We're going to need a better plan if we want to stop using the backend to render whole units. To be clear, this is not to say that this PR is wrong. We might find a frontend-only solution to the performance issues. But we should probably hold off here until we've had that conversation. |
FYI @ormsbee @Agrendalath |
|
Thank you for sharing it, @arbrandes. In this case, I suppose we should close this and openedx/frontend-app-learning#1281 for now. cc: @ormsbee, @mphilbrick211 |
|
@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. |
Rendering blocks directly in the MFE was developed for the Library Authoring MFE. The solution was then ported to the Learning MFE. This commit includes backend changes required to retrieve data of non-blockstore XBlocks from LMS.
Please see openedx/frontend-app-learning#1281 for the details and testing instructions.
Other information