-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Removed jquery-ui-bundle #13966
Removed jquery-ui-bundle #13966
Conversation
This pull request has been linked to Shortcut Story #24120: Figure out why we still need jQuery-ui-bundle. |
PR Summary
|
Friendly ping @snipe @uberbrady |
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.
Pulling stuff out of package.json
is a universal GOOD. And Red diffs are the BEST diffs! But, besides that, I think that removing a package that even just has the appearance of security issues is important, all-around. I'm going to want to make sure that those checksum changes will still allow for installs to work correctly and don't introduce any new problems, and I'll want to take a quick breeze through the app to make sure everything still looks good, but I think this is great! Thanks for tracking all of this down, and getting this out there. I'm going to try and look at it today.
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.
I had to resolve the conflicts in package-lock.json to get this to work, but I was able to run through all the main pages of the app and saw no JS errors. I think this is good! Thanks again for the fix!
Description
This PR removes
jquery-ui-bundle
and replaces it with the existingjquery-ui
javascript.Removing
jquery-ui-bundle
, which was last updated to 1.12.1, and usingjquery-ui
directly means that, even though the project is in maintenance mode, we can bump to 1.13.2 and receive any additional fixes in the future.The removal without replacement of the
"./node_modules/jquery-ui-bundle/jquery-ui.css"
line doesn't seem to have an affect on the front end. I believe this is because AdminLTE also requiresjquery-ui
. This probably means we could remove it from ourpackage.json
but I quickly tried it and broke javascript. Let me know if I should explore it more.I went through the process of juggling branches while building assets to make sure nothing broke but please double-check my work.
Type of change