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

Update /core/misc/jquery.form.js to the latest version (4.2.1). #2574

Closed
klonos opened this issue Mar 15, 2017 · 11 comments
Closed

Update /core/misc/jquery.form.js to the latest version (4.2.1). #2574

klonos opened this issue Mar 15, 2017 · 11 comments

Comments

@klonos
Copy link
Member

klonos commented Mar 15, 2017

This has not been updated since 2014.

New GitHub home of the project: https://github.com/jquery-form/form
Direct link to the minified file: https://github.com/jquery-form/form/raw/master/dist/jquery.form.min.js
Release log: https://github.com/jquery-form/form/releases


PR by @klonos: backdrop/backdrop#1805 (4.1.0)
PR by @klonos: backdrop/backdrop#1816 (4.2.0)
PR by @klonos: backdrop/backdrop#1822 (4.2.1)

@quicksketch
Copy link
Member

Thanks @klonos! Great idea to update this library. This should go into the next minor release (1.7.0) as it will likely add new APIs for that version of the library.

In the PR, don't forget to update the version number in system_library_info().

We'll also need verification of several manual JavaScript-related tasks: Uploading a file, the "Add more" button for fields, and the Views AJAX-based exposed filters seem to be the primary places where we use this library.

@klonos klonos added this to the 1.7.0 milestone Mar 19, 2017
@klonos
Copy link
Member Author

klonos commented Mar 19, 2017

This should go into the next minor release (1.7.0)...

Set the milestone so it doesn't slip under the radar 😉

In the PR, don't forget to update the version number in system_library_info().

Updated the PR. I went with the GitHub project URL (https://github.com/jquery-form/form) for 'website' because IMO it is less likely to change in the future, but there's also the "official" website https://jquery-form.github.io/form. Let me know if you want me to change to that.

We'll also need verification of several manual JavaScript-related tasks: Uploading a file, the "Add more" button for fields, and the Views AJAX-based exposed filters seem to be the primary places where we use this library.

Setting the PR to NR for the code changes part. Feel free to change back to NW and I'll get to that soon as I have more time.

@klonos klonos changed the title Update /core/misc/jquery.form.js to the latest version (4.1.0). Update /core/misc/jquery.form.js to the latest version (4.2.0). Mar 20, 2017
@klonos
Copy link
Member Author

klonos commented Mar 20, 2017

New version of the library released. New PR upcoming...

@klonos
Copy link
Member Author

klonos commented Mar 29, 2017

This is targeted for 1.7.0, so there's still time, but before merging we should keep an eye on https://www.drupal.org/node/2860158 and jquery-form/form#521 (and by "we" I mean "I" 😄 ) ...plus test everything that @quicksketch suggested in #2574 (comment) and everything that users in the d.org issue reported broken.

@klonos
Copy link
Member Author

klonos commented Mar 29, 2017

...it seems that in d.org they are using jQuery form 4.0.1 and they try to mitigate the issues by conditionally loading that version if jQuery >= 1.8 or 1.9. We on the other hand are using 1.11.0, so the d.org issue might not apply to Backdrop 😉 ...we should still check though just in case..

@kevindb
Copy link

kevindb commented Mar 29, 2017

This issue being discussed on d.org was a regression in jQuery Form that caused spaces in the <input type="submit"> name to be replaced with plus signs (+) during form serialization. This regression was present regardless of the version of jQuery used.

jQuery Form has been updated to v4.2.1 with fix from @MarkCarver.

Form v4 is compatible with jQuery 1.7 through 1.11. Form v4.2.0 added support for jQuery 1.12+ and jQuery 2 and 3.
I would, of course, recommend using the latest version of jQuery Form! 😁

@klonos
Copy link
Member Author

klonos commented Mar 29, 2017

Thanx for taking the time to explain @kevindb. New PR with the updated 4.2.1: backdrop/backdrop#1822

@quicksketch do you think that it's a good idea to be including jquery.form.min.js.map along with the minified library for better DX? Let me know and I'll add it.

@klonos klonos changed the title Update /core/misc/jquery.form.js to the latest version (4.2.0). Update /core/misc/jquery.form.js to the latest version (4.2.1). Mar 30, 2017
@quicksketch
Copy link
Member

do you think that it's a good idea to be including jquery.form.min.js.map along with the minified library for better DX? Let me know and I'll add it.

I think we can go without and be just fine. It's pretty uncommon in my own experience to be debugging directly within jQuery or a jQuery plugin. A developer could always drop the missing files in if truly needed, though most devs I've seen faced with this issue would just drop a completely uncompressed version into the same place. Not fancy but effective.

With the Drupal community already having tested 4.2.1 and my own validation in the test sandbox showing no issues, I think this is good for merge.

I've merged backdrop/backdrop#1822 into 1.x for the next minor release, 1.7.0.

Thanks @klonos for the PR. And wow @kevindb the jQuery Forms maintainer; here checking in!! Awesome! Great work maintaining the library. I had a hand in putting it in Drupal ages ago, it's fantastic and unusual to have such longevity in a library. Kudos to you.

@klonos
Copy link
Member Author

klonos commented Apr 13, 2017

@quicksketch ...while in my previous 2 PRs for v 4.1.0 and 4.2.0 I did update the respective entry in system.module, I failed to do so in the last PR (the one that got merged). Here's a follow-up PR for it: backdrop/backdrop#1833

Sorry 😄

@quicksketch
Copy link
Member

No problem! Thanks I should have caught that. Merged the follow-up at backdrop/backdrop#1833 for 1.7.0.

@klonos
Copy link
Member Author

klonos commented Apr 13, 2017

Thanks I should have caught that.

...and I should have gotten it right in the first place. No worries, we're only human.

@klonos klonos closed this as completed Apr 13, 2017
@klonos klonos removed their assignment May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants