-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fix optimizing of draggabilly and json2 #10534
Conversation
@@ -1748,7 +1748,9 @@ return Unidragger; | |||
|
|||
if ( typeof define == 'function' && define.amd ) { | |||
// AMD | |||
define( [ | |||
define( |
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.
Reviewers: this is the key problem which is that the Draggabilly vendor file doesn't correctly define its own module when using RequireJS. This fix allows the file to be bundled by RequireJS Optimizer.
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.
Please add a comment to the code explaining why this is needed and that it is specifically for Studio (since LMS uses a namespaced version).
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.
Done
@cahrens I have no idea what is going on, but I can't log in to Studio on my sandbox. I can't see how my changes could have caused this, although I suppose they must have. There are no errors in the browser, and no errors shown in the cms log file. |
I updated my sandbox to the RC branch and am having the same trouble, so it isn't my change. I've kicked off a clean sandbox build in case something went wrong with the last one. |
At @cahrens's suggestion, I've removed the JSON2 shim from Studio as it is not needed. The JSON2 library is only used for older browsers, none of which we support. |
2c9a16a
to
a8f84be
Compare
a8f84be
to
7a14696
Compare
@cahrens @dianakhuang Please review this fix for this week's release. |
FYI you can see a video rendering successfully in Studio on my sandbox: |
👍 |
👍 yay for getting rid of things we don't need. |
jenkins run python |
jenkins run lettuce |
jenkins run bokchoy |
jenkins run lettuce |
2 similar comments
jenkins run lettuce |
jenkins run lettuce |
Fix optimizing of draggabilly and json2
Proposed fix for the release candidate for https://openedx.atlassian.net/browse/TNL-3766
I've kicked off a sandbox build so I'll see how that looks in the morning:
https://andy-armstrong.sandbox.edx.org
FYI: @cahrens @jcdyer