fix: remove error ReferenceError: JavascriptLoader is not defined in Zoom Image Tool #35661
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @jignaciopm! 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. |
mphilbrick211
added
needs test run
|
Hello @arbrandes and @feanil I'm tagging you as the reporter and assignee of #31436. |
There was a problem hiding this comment.
Thanks for the PR! It indeed fixes the loading of the loupe script.
However, the version in the default content is broken, and there's no way to fix it except by hosting a version of the script somewhere ourselves. Which brings us back around to the question: should we be supporting something like this officially?
My personal opinion is that unless we're ready to make the default content work properly by including a version of the loupe script with the HTML block, we should just remove this template. I'm going to confirm this with Product.
| let script = document.createElement("script"); | ||
| script.type = "text/javascript"; | ||
|
|
||
| if (script.readyState) { // IE |
There was a problem hiding this comment.
We no longer support IE, so this conditional can be removed in favor of script.onload.
| let script = document.createElement("script"); | ||
| script.type = "text/javascript"; | ||
|
|
||
| if (script.readyState) { // IE |
There was a problem hiding this comment.
Same as above. We can do with only script.onload, now.
| }); | ||
| } | ||
|
|
||
| loadScript("https://studio.edx.org/c4x/edX/DemoX/asset/jquery.loupeAndLightbox.js", callback); |
There was a problem hiding this comment.
The problem here is that this version of the loupe is broken when you use it on a full-width, desktop LMS. The positioning is off:
There's no way to fix it except by modifying the source of the script. And here lies the major problem I have with this template: by default it depends on a script that's hosted by edx.org, and a buggy one at that.
|
@jignaciopm thanks for the PR it prompted much discussion and led to #36012 which should resolve the issue and be a bit easier to maintain going forward. |
Description
This PR uses a workaround to load the script jquery.loupeAndLightbox.js, which waits for the script to be completely loaded in the browser before running any reference to its utilities.
Useful information to include:
.zooming-image-place .largerin frontend-app-learningLMS In Firefox:
CMS in Firefox:
LMS in Chrome:
CMS in Chrome:
Supporting information
[DEPR] HTML Block: The Zooming Image Tool template is broken: #31436
Testing instructions
Deadline
None
Other information
None