-
Notifications
You must be signed in to change notification settings - Fork 209
chore: upgrade jQuery UI to version 1.14.1 #2314
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the pull request, @luisfelipec95! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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. 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2314 +/- ##
==========================================
+ Coverage 95.09% 95.29% +0.19%
==========================================
Files 195 195
Lines 21479 21664 +185
Branches 1931 1509 -422
==========================================
+ Hits 20425 20644 +219
+ Misses 787 771 -16
+ Partials 267 249 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The failure in the workflow is related to the minified jQuery UI file (jquery-ui-1.14.1.min.js), which appears to contain tokens with strange or empty characters. This is causing make check_translations_up_to_date to fail during the extraction phase. As you can see, the extracted strings are likely artifacts of parsing the minified file, which shouldn't normally be included in the translation workflow. I also tried running the commands from the Makefile manually to update the translations, but it still fails because of this. Does anyone know what’s the expected process in this case? Any guidance would be appreciated. Thanks! @openedx/content-aurora |
|
@luisfelipec95 Thank you for your contribution! I'm marking it as ready for review by the @openedx/2u-aurora As a first step could you please have a look at @luisfelipec95's questions above? @feanil It looks like the legacy |
|
We should drop the old |
@feanil Thanks! -- @openedx/2u-aurora Friendly reminder about this -- could you please let @luisfelipec95 know when you'd be able to reply to the questions they raised earlier? |
|
Hi @openedx/committers-edx-ora2! This PR needs some technical input (see #2314 (comment)), could you please have a look? |
|
@openedx/committers-edx-ora2 A friendly reminder to help out @luisfelipec95 with his questions. |
|
@luisfelipec95 Sorry for the delay here. I reached out on Slack to find a reviewer from the wider CC community. |
|
Hey @openedx/axim-engineering team, could you please have a look at this PR and help @luisfelipec95 with his questions? It's been a while since this PR was created and so far, my attempts at getting responses from maintainers and/or CCs have been unsuccessful. |
|
My first thought was that we had a specific exclude in place for When investigating this locally I found out we were getting away with not ignoring vendored minified js. To figure out what was going on I updated Line 97 in 801fbd1
to use in the output. I pulled down this PR branch and rebased it on latest master locally (I encountered errors running I updated the diff --git a/Makefile b/Makefile
index 6be23a42a..20cd58830 100644
--- a/Makefile
+++ b/Makefile
@@ -94,7 +94,7 @@ static: ## Webpack JavaScript and SASS source files
extract_translations: ## creates the django-partial.po & django-partial.mo files
cd ./openassessment && django-admin makemessages -l en -v1 -d django
- cd ./openassessment && django-admin makemessages -l en -v1 -d djangojs
+ cd ./openassessment && django-admin makemessages -l en -v1 -d djangojs -i "*jquery-ui*.min.js"
compile_translations: ## compiles the *.po & *.mo files
cd ./openassessment/ && i18n_tool generate -v && cd ..and then I was able to successfully run I specifically chose to limit what was being ignored to only the That being said, I think it probably makes sense for
OR
|
|
Thanks a lot @brian-smith-tcril! @luisfelipec95 Does that give you what you need to move forward here? ⬆️ |
Why this change?
This PR updates the local jQuery UI file (
openassessment/xblock/static/js/lib/jquery-ui-1.10.4.min.js) from version 1.10.0 (2013) to version 1.14.1(openassessment/xblock/static/js/lib/jquery-ui-1.14.1.min.js) (2024), the latest stable release from the official jQuery UI project.Motivation
For full changelog:
https://jqueryui.com/changelog/
https://jqueryui.com/upgrade-guide/1.14/