-
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
feat: remove unused JS requirement in CMS #34545
feat: remove unused JS requirement in CMS #34545
Conversation
Thanks for the pull request, @bradenmacdonald! 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. |
@bradenmacdonald @brian-smith-tcril - is this still in progress? |
@mphilbrick211 I would still like to merge this, if @brian-smith-tcril or maybe @kdmccormick could review it. |
66464a3
to
5f5d0ef
Compare
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.
Thanks Braden!
nit: you could merge this in as a |
5f5d0ef
to
d7c436c
Compare
I'm planning to merge this on Tuesday. |
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
This reverts commit 4a12de7.
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
In the legacy Studio UI's JS console on a tutor devstack, a couple 404 errors can be seen:
I believe this error has been there for a long time, without causing any issues. That is, the legacy Studio UI does not seem to depend on the Bootstrap (or popper) JS.
Some LMS legacy UI pages do depend on bootstrap+popper, which they get from
edx-platform/lms/templates/main.html
Lines 117 to 120 in 68b3753
if needed. This file is not part of the repo's source code, but is created by
edx-platform/scripts/copy-node-modules.sh
Line 58 in 68b3753
Testing instructions
You can go to the "bookmarks" page of a course in the legacy LMS UI (which is a page that uses bootstrap's JS), e.g. http://local.overhang.io:8000/courses/course-v1:A+B+C/bookmarks/ and in the JS console you can run
$.fn.modal
and$.fn.carousel
to verify that Bootstrap's JS is active.But, with or without the changes in this PR, if you run those same commands in any legacy Studio UI page, you'll see that Bootsrap's JS is not loaded, but the UI works fine.
Likewise on any production instance, you won't see the 404 error because popper/bootstrap are simply missing from the generated bundle
cms-base-vendor.x.js
. But you can open a JS console and verify that e.g.$.fn.modal
and$.fn.carousel
are not loaded on the page.Deadline
None
Other information