Minimal Library Asset Support #1383
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1383 +/- ##
==========================================
+ Coverage 93.01% 93.04% +0.02%
==========================================
Files 1035 1043 +8
Lines 19632 19956 +324
Branches 4172 4223 +51
==========================================
+ Hits 18261 18568 +307
- Misses 1306 1326 +20
+ Partials 65 62 -3 ☔ View full report in Codecov by Sentry. |
Ian2012
approved these changes
Oct 15, 2024
kdmccormick
approved these changes
Oct 15, 2024
| // because the code currently just doesn't handle it correctly. Libraries don't mangle the path | ||
| // into an asset key–it might be sufficient to remove the initial "/" in a "/static/images/foo.png" | ||
| // link, and then set the base URL to the correct ComponentVersion base. | ||
| if (!learningContextId.startsWith('lib:')) { |
There was a problem hiding this comment.
[nit]
Suggested change
| if (!learningContextId.startsWith('lib:')) { | |
| if (!isLibraryKey(learningContextId)) { |
There was a problem hiding this comment.
+1, we have key helper methods in src/generic/key-utils.ts
There was a problem hiding this comment.
I hadn't realized. Will do.
| // into an asset key–it might be sufficient to remove the initial "/" in a "/static/images/foo.png" | ||
| // link, and then set the base URL to the correct ComponentVersion base. | ||
| if (!learningContextId.startsWith('lib:')) { | ||
| // assets in expandable text areas so not support relative urls so all assets must have the lms |
There was a problem hiding this comment.
Suggested change
| // assets in expandable text areas so not support relative urls so all assets must have the lms | |
| // assets in expandable text areas do not support relative urls so all assets must have the lms |
not your typo, but so->do
bradenmacdonald
approved these changes
Oct 15, 2024
ormsbee
commented
Oct 16, 2024
| // So until we handle it better, just disable static asset URL substitutions | ||
| // when dealing with Library content. | ||
| return false; | ||
| } |
There was a problem hiding this comment.
@bradenmacdonald, @kdmccormick: I changed this to be a simpler early return–since I realized that this function returns false if it finds no matching static URLs.
edx-platform commit 7316111 (PR #35598) moved the XBlock embed view so that it can be rendered on either LMS or Studio. This commit moves the frontend to actually call the Studio endpoint. This will make Content Library static asset display easier, because that view will only be made available through Studio and not the LMS.
The static asset substitution used to make images show up properly when in the TinyMCE editor doesn't work for Content Libraries. Unfortunately, this will cause the static asset references in XBlock content to get mangled and saved incorrectly. So until we can handle it correctly, we're just going to disable it entirely if the LearningContext is a v2 Content Library. This means that static assets won't display properly in the editor itself, but it should at least get written/preserved correctly, so that those assets will show up properly in XBlock previews.
ormsbee
force-pushed
the
minimal-asset-support
branch
from
0cfac35 to
234496b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a subset of #1349 that doesn't have all the Files & Uploads changes, but just has: