Skip to content
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

Upgrade jQuery to 3.6.0. #3442

Merged
merged 1 commit into from
Nov 16, 2021
Merged

Upgrade jQuery to 3.6.0. #3442

merged 1 commit into from
Nov 16, 2021

Conversation

dracos
Copy link
Member

@dracos dracos commented May 21, 2021

Upgrading to jQuery 2.2.4 (+3.5.0 security patch) was done in #3017 after I took this commit out so the change wasn't as big.

I think only a couple of changes here are actually needed (e.g. complete to always), the others are warnings given by jquery migrate but still work fine, so if any have been missed it shouldn't matter (hopefully haven't though). The :first/:last/etc selectors were deprecated in 3.4 but still working (currently scheduled to be removed in 4, I think). But e.g. first() is quicker than :first so no harm in doing these. click() etc were technically deprecated in 3.3 - https://blog.jquery.com/2018/01/19/jquery-3-3-0-a-fragrant-bouquet-of-deprecations-and-is-that-a-new-feature/ - but the docs still don't say they are and there are frequent issues on the migrate repo saying why does it warn and they say "PR the docs" and have been since at least 2018, so who knows what's happening there. Again, no harm in switching now though.

@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #3442 (d665128) into master (af6eed1) will decrease coverage by 10.42%.
The diff coverage is 79.80%.

❗ Current head d665128 differs from pull request most recent head 6063020. Consider uploading reports for the commit 6063020 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3442       +/-   ##
===========================================
- Coverage   82.64%   72.21%   -10.43%     
===========================================
  Files         345       51      -294     
  Lines       23662     4384    -19278     
  Branches     3608        0     -3608     
===========================================
- Hits        19555     3166    -16389     
+ Misses       2973     1218     -1755     
+ Partials     1134        0     -1134     
Impacted Files Coverage Δ
web/cobrands/fixmystreet/offline.js 45.02% <0.00%> (ø)
web/js/duplicates.js 62.50% <0.00%> (ø)
web/js/map-OpenLayers.js 77.24% <25.00%> (-0.17%) ⬇️
web/cobrands/fixmystreet/fixmystreet.js 73.84% <84.93%> (+0.64%) ⬆️
web/cobrands/fixmystreet/admin.js 18.10% <87.50%> (ø)
web/cobrands/fixmystreet/staff.js 55.95% <91.66%> (ø)
web/cobrands/bromley/a-z-nav.js 50.00% <100.00%> (ø)
web/cobrands/fixmystreet.com/js.js 72.72% <0.00%> (-9.10%) ⬇️
web/cobrands/fixmystreet-uk-councils/alloy.js 88.11% <0.00%> (-2.98%) ⬇️
... and 297 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af6eed1...6063020. Read the comment docs.

@dracos
Copy link
Member Author

dracos commented May 24, 2021

@zarino you already reviewed this last year, and the 2.2 part of that is live now, so this is just the 3 changes, which aren't anything much it seems.

Copy link
Member

@zarino zarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I notice lots of changes inside web/vendor/fancybox/jquery.fancybox-1.3.4.js. Should we just be using the latest version (3.5.7) of fancybox instead?

@dracos
Copy link
Member Author

dracos commented May 24, 2021

That version is only available under a non-commercial licence. I think on the old PR I did say someone should go and evaluate all the new JS packages out there and pick the smallest new one to switch to :)

@zarino
Copy link
Member

zarino commented May 24, 2021

Yup, not a dealbreaker. Just thought I’d check we had a reason for not using the latest. Agree it’s something we can sort in a subsequent PR.

@dracos dracos merged commit 6063020 into master Nov 16, 2021
@github-pages github-pages bot temporarily deployed to github-pages November 16, 2021 12:10 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants