OpenTreeMap / otm-core

OpenTreeMap is a collaborative platform for crowdsourced tree inventory, ecosystem services calculations, urban forestry analysis, and community engagement.
www.opentreemap.org
Other
186 stars 88 forks source link

Upgrade jQuery to 3.3.1 #3229

Closed jwalgran closed 6 years ago

jwalgran commented 6 years ago

Overview

Update jQuery to the latest version and jquery.fileupload to a newer version.

Connects #3228 Required by https://github.com/OpenTreeMap/otm-addons/pull/1553

Notes

I tried to update jquery.fileupload and its companion libraries to the latest versions, but it caused a webpack error due to a change in AMD require naming.

blueimp/jQuery-File-Upload@a9bfde1

I pulled in the version before this naming change (9.12.4) and it resolved the webpack issue along with eliminating the deprecation warning from jquery-migrate.

Similarly I attempted use the latest jquery-migrate (3.0.1) but ran into webpack AMD errors. Rolling back to version 3.0.0 as mentioned in Github issue threads resolved the issue.

Some of our dependencies use deprecated methods. Since the methods are still available in jQuery 3.x I opted to not resolve them via updates. In some cases (typeahead) there is no compatible version to update to.

Testing Instructions

The scope of this change is application-wide. This PR needs more of a sanity-check than a full interactive review because it will go through a full round of QA before it is released to production.

I ran through the relevant section of our QA checklist as documented in https://github.com/OpenTreeMap/otm-core/issues/3228

These are the hotspots I tested that were directly affected by upgrade changes.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 83.867% when pulling 05da77c1baea758e720ca9656e308c7fb6da3a85 on jcw/jquery-3 into 8f5108e8a90554a55d9a6e06db1372953975b028 on develop.

jwalgran commented 6 years ago

@azavea-bot rebuild

jwalgran commented 6 years ago

@azavea-bot rebuild

jwalgran commented 6 years ago

@azavea-bot rebuild

tnation14 commented 6 years ago

@azavea-bot rebuild

rajadain commented 6 years ago

This .bind() should be turned into a .on():

https://github.com/OpenTreeMap/otm-core/blob/185a5394fab282cefb5e0f136dd88d9a62bf5f53/opentreemap/treemap/js/src/lib/uploadPanel.js#L83

All the others I could find were dependencies:

> ag -l --js '[^_]\.bind\('

assets/js/shim/ion.rangeSlider.js
assets/js/shim/jquery.fileupload.js
assets/js/shim/leaflet.utfgrid.js
assets/js/shim/jscolor.js
assets/js/shim/jquery.iframe-transport.js
assets/js/shim/typeahead.jquery.js
assets/js/vendor/jquery.ui.widget.js
assets/js/vendor/jquery.js
opentreemap/treemap/js/src/lib/uploadPanel.js
> ag -l --js '[^_]\.unbind\('

assets/js/shim/jquery.iframe-transport.js
assets/js/vendor/jquery.ui.widget.js
assets/js/shim/jquery.fileupload.js
rajadain commented 6 years ago

The event shorthands $().click(HANDLER) and $().click() have been deprecated in favor of .on('click', HANDLER) and .trigger('click'): https://github.com/jquery/jquery-migrate/blob/master/warnings.md#jqmigrate-jqueryfnclick-event-shorthand-is-deprecated

But their instances still remain:

> for i in click blur focus focusin focusout resize scroll dblclick mousedown mouseup mousemove mouseover mouseout mouseenter mouseleave change select submit keydown keypress keyup contextmenu; ag --js "\.$i\("; end

modeling/js/src/modeling.js
243:    $(dom.staleRevisionDialogOk).click(function () {

treemap/js/src/mapPage/locationSearchUI.js
36:    $(dom.clearLocationInput).click(clearLocationInput);
37:    $(dom.clearCustomArea).click(clearCustomArea);

treemap/js/src/mapPage/editAreaMode.js
31:    $(dom.editArea).click(modes.activateEditAreaMode);
32:    $(dom.saveArea).click(saveArea);
33:    $(dom.cancelEdit).click(cancelEditing);

treemap/js/src/mapPage/drawAreaMode.js
26:    $(dom.drawArea).click(modes.activateDrawAreaMode);
27:    $(dom.cancel).click(cancelDraw);

treemap/js/src/treeMap.js
56:$('[data-action="addtree"]').click(function(e) {
60:$('[data-action="addresource"]').click(function(e) {

treemap/js/src/lib/socialMediaSharing.js
78:    $(dom.toggle).click(function () {

treemap/js/src/lib/utility.js
223:            link.click();

treemap/js/src/lib/mapFeatureDelete.js
22:    $(dom.deleteConfirm).click(function () {
38:    $(dom.deleteCancel).click(function () {
41:    $(dom.delete).click(function () {

treemap/js/src/lib/diameterCalculator.js
197:    $parentForm.find(_addRowBtnSelector).click(

treemap/js/src/lib/geometryMover.js
30:    $editLocationButton.click(function () {
37:    $cancelEditLocationButton.click(function () {

treemap/js/src/lib/mapFeatureUdf.js
39:        $tables.find('.resolveBtn').click(markTargetAsResolved);
61:    $('a[data-udf-id]').click(function() {

treemap/js/src/lib/buttonEnabler.js
52:        $el.click(function () {

manage_treemap/js/src/branding.js
42:$(dom.useDefaultColors).click(function () {
treemap/js/src/lib/searchBar.js
151:        $advancedToggle.toggleClass('active', active).blur();

treemap/js/src/lib/otmTypeahead.js
238:            $input.blur();
treemap/js/src/simpleForm.js
6:    $('input[type!="hidden"]').first().focus();

treemap/js/src/lib/utility.js
235:        $(this).find('input').first().focus().select();

treemap/js/src/lib/popover.js
33:    $(dom.popup.input).focus().select();

manage_treemap/js/src/embed.js
32:        $snippet.focus().select();

otm_comments/js/src/lib/comments.js
46:        $('#id_comment').focus();
modeling/js/vendor/Chart.js
3870:           this.resize(true);
5891:               me.controller.resize();
manage_treemap/js/src/udfs.js
461:        $el.change();
modeling/js/src/lib/modelingParamUI.js
160:        $row.find('input').first().select();

treemap/js/src/lib/utility.js
235:        $(this).find('input').first().focus().select();

treemap/js/src/lib/popover.js
33:    $(dom.popup.input).focus().select();

manage_treemap/js/src/embed.js
32:        $snippet.focus().select();
rajadain commented 6 years ago

Other than the two comments above, this code looks good! I'm not sure how necessary it is to remove the event shorthands. They are mentioned in the jQuery Migrate documentation, but not in the jQuery 3.0 Upgrade Guide.

jwalgran commented 6 years ago

Thanks for that handy silver search. I think it is worth it to make these event handler changes now while we are in here.

jwalgran commented 6 years ago

Rebased with the suggested fixes. Will merge after the CI build passes. Thanks for the thorough inspection.