fix: allow to preview images #1403
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @Ian2012! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by 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. |
Ian2012
force-pushed
the
cag/library-image-preview
branch
4 times, most recently
from
ca9c6f1
to
68a673c
Compare
Ian2012
force-pushed
the
cag/library-image-preview
branch
4 times, most recently
from
494f6d2
to
6a52f3c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1403 +/- ##
==========================================
+ Coverage 93.15% 93.16% +0.01%
==========================================
Files 1051 1053 +2
Lines 20328 20413 +85
Branches 4335 4280 -55
==========================================
+ Hits 18936 19018 +82
- Misses 1332 1335 +3
Partials 60 60 ☔ View full report in Codecov by Sentry. |
Ian2012
force-pushed
the
cag/library-image-preview
branch
3 times, most recently
from
9ffa8d0
to
b55811d
Compare
| // This function doesn't currently know how to deal with Library assets. | ||
| // 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 we let this | ||
| // function try to deal with Library assets, it would convert them in such a | ||
| // way that it wouldn't convert back later on, and we'd end up storing the | ||
| // incorrect OLX and breaking the display from that point forward. | ||
| // | ||
| // So until we handle it better, just disable static asset URL substitutions | ||
| // when dealing with Library content. |
There was a problem hiding this comment.
This is no longer accurate, right? Now that your code is dealing with the case properly?
There was a problem hiding this comment.
Yes, I'll update this comment with the details of the solution
|
I think the code looks fine. I defer to you folks on whether this is impractical to test because of TinyMCE. I'll approve and merge as soon as the comments are updated, unless @bradenmacdonald has additional feedback to give. |
|
I'm gonna test this in a few minutes. |
|
✅ This works well for existing images, but I don't see any option to upload a new image. Was that out of scope? |
| @@ -261,9 +262,12 @@ export const editorConfig = ({ | |||
| content, | |||
| minHeight, | |||
| learningContextId, | |||
| documentURL, | |||
There was a problem hiding this comment.
documentUrl doesn't mean much to me. staticRootUrl would be more clear
|
LGTM, no blockers from me. Just needs updated comments. |
|
@Ian2012 @ormsbee Not sure what's wrong yet, but this PR is deployed to the sandbox, yet it's not working on there:
The exact commit running on the sandbox at the moment is https://github.com/open-craft/frontend-app-authoring/commits/taxonomy-sandbox-20241022/ if that helps. The URL should be rewritten to https://cms.tagging-preview.staging.do.opencraft.hosting/library_assets/blocks/lb:OpenCraft:TL:html:cc695559-2cff-43ab-a463-02346b7ab9b8/static/deer.jpg which is working |
|
@bradenmacdonald: The usage key is undefined. It shows it trying to load this for me: |
|
@ormsbee Ah, hmm, you're right. Now I see it has to do with a recent refactor on Though we have to fix it anyways because even without that conflict, if you use the "Edit" menu to open the editor without opening the sidebar, you get the same problem: |
Ian2012
force-pushed
the
cag/library-image-preview
branch
5 times, most recently
from
95e7014
to
042c4ba
Compare
fix: dinamically get the component UUID chore: quality fixes chore: quality fixes chore: quality fixes chore: quality fixes fix: add optional library context hook chore: update comment fix: add blockID parameter to textEditor
Ian2012
force-pushed
the
cag/library-image-preview
branch
2 times, most recently
from
47000cb
to
9e8fe74
Compare
Ian2012
force-pushed
the
cag/library-image-preview
branch
2 times, most recently
from
4d013d7
to
3d1ec42
Compare
Ian2012
force-pushed
the
cag/library-image-preview
branch
from
3d1ec42
to
9bb02c6
Compare
Description
This PR allows to preview images on the text editor for Libraries.
Supporting information
Closes #1358
Depends on openedx/edx-platform#35681
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Screenshots