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.

Sign up for GitHub

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

Jump to bottom

Remove jQuery from track-radio-group.js #2270

Merged
merged 4 commits into from
Nov 11, 2021
Merged

Remove jQuery from track-radio-group.js #2270

merged 4 commits into from
Nov 11, 2021

Conversation

gclssvglx
Copy link
Contributor

@gclssvglx gclssvglx commented Nov 5, 2021 •
edited
Loading

What

This change removes jQuery from the track-radio-group.js module and replaces it with vanilla JS

Why

We're removing jQuery from GOV.UK

Trello

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch 2 times, most recently from 52810a1 to 05d71e1 Compare November 8, 2021 10:33
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 8, 2021 10:33 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 05d71e1 to 9fcd898 Compare November 9, 2021 08:43
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 9, 2021 08:43 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 9fcd898 to 8c4b7cc Compare November 9, 2021 08:46
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 9, 2021 08:46 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 8c4b7cc to cc86f15 Compare November 9, 2021 15:31
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 9, 2021 15:32 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 10, 2021 10:55 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from edafdef to 3b39f7f Compare November 10, 2021 13:05
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 10, 2021 13:05 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 3b39f7f to b24b5e7 Compare November 10, 2021 13:08
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 10, 2021 13:09 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from b24b5e7 to 265828b Compare November 10, 2021 14:24
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 10, 2021 14:24 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 10, 2021 15:39 Inactive
@gclssvglx
Remove jQuery from track-radio-group.js (except the Ajax request)
bafb4eb 
This change removes - most - of the jQuery from the track-radio-group.js
module and replaces it with vanilla JS. It doesn't replace the '$.ajax()'
function call - that will happen in a separate commit.

It was noted when replacing the 'track' event handler for 'submit' that the
tests became broken. This was put down to jQuery handling 'trigger' differently
now that it is a DOM element. A change to triggering 'click' on the forms
submit button resolved the issue, but caused an error where the form was no
longer attached to the document. Appending it in beforeEach() and removing it in
afterEach() resolved the issue.
@gclssvglx
Remove jQuery Ajax request from track-radio-group.js
601f4d7 
The jQuery '$.ajax()' method used is a [JSON-P (JSON with Padding)][1] request.
It is not however, an Ajax request as it [does not use the 'XMLHttpRequest' object][2].

This is indicated by the use of the 'dataType: 'jsonp'' config option when
calling the jQuery function e.g. '$.ajax({dataType: 'jsonp'})'.

To achieve this ["jQuery attaches a global function to the window object that is
called when the script is injected, then the function is removed on completion."][3].

Essentially, jQuery creates a '<script>' element with a 'src' attribute set to
the URL of the request. A 'callback' function is also created and it's name is
appended to the URL.

When the request is made successfully - the specified 'callback' function is
called with the data returned from the request as the first (and only)
parameter. The '<script>' element is then removed.

This technique allows JavaScript to send data to-and-from foreign origins which
would otherwise fail due to a [CORS (Cross-Origin Resource Sharing)][4] policy.

We need to mimic this behaviour in vanilla JavaScript - and within a module.

Firstly, we add an event trigger that is attached to the 'window.GOVUK' context.
This is our 'callback' function and will signal the completion of the JSON-P
request.

Next we create an event listener for the above trigger which will get the
response data from the event and call the appropriate function inside the
module.

Finally, the jQuery '$.ajax()' function can be replaced by a function that now
creates the '<script>' element and assigns a suitable 'src' - complete with
'callback' name. A 'timestamp' is used to avoid caching.

[1]: https://en.wikipedia.org/wiki/JSONP
[2]: https://www.w3schools.com/js/js_json_jsonp.asp
[3]: https://remysharp.com/2007/10/08/what-is-jsonp/#how-it-works-in-jquery
[4]: https://en.wikipedia.org/wiki/Cross-origin_resource_sharing
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 7d8ba17 to 9dc338e Compare November 10, 2021 15:52
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 10, 2021 15:52 Inactive
@gclssvglx
Remove div wrapper around form in tests
e283679 
The TrackRadioGroup module is only initialised with a form element in the
production code. However, the tests wrap the form in a div.

We should remove the div wrapper to simplify the tests and the code.
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 9dc338e to e283679 Compare November 10, 2021 15:55
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 10, 2021 15:55 Inactive
@gclssvglx gclssvglx marked this pull request as ready for review November 10, 2021 16:09
andysellick
andysellick approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Great work on this @gclssvglx 👏

Couple of suggestions on making what this does a bit clearer, but otherwise looks good.

app/assets/javascripts/modules/track-radio-group.js Show resolved Hide resolved
app/assets/javascripts/modules/track-radio-group.js Show resolved Hide resolved
app/assets/javascripts/modules/track-radio-group.js Show resolved Hide resolved
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 09:25 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 7b18cc3 to 5ecb89f Compare November 11, 2021 10:43
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 10:44 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 5ecb89f to f1bf70c Compare November 11, 2021 11:07
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 11:07 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from f1bf70c to 4502379 Compare November 11, 2021 12:55
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 12:55 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 4502379 to e89bae8 Compare November 11, 2021 12:57
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 12:58 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from e89bae8 to 7845dfd Compare November 11, 2021 13:04
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 13:04 Inactive
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from 7845dfd to ee94412 Compare November 11, 2021 13:12
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 13:12 Inactive
@gclssvglx
General clean up
33da2f8 
The event options merge is simplified as it logic inside 'crossDomainTrackingEnabled'

Comments are added to a couple of functions to aid understanding
@gclssvglx gclssvglx force-pushed the remove-jquery-from-track-radio-group branch from ee94412 to 33da2f8 Compare November 11, 2021 13:14
@govuk-ci govuk-ci temporarily deployed to government-f-remove-jqu-jtk0v8 November 11, 2021 13:14 Inactive
andysellick
andysellick approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Excellent changes and work on this @gclssvglx. Think it's ready now. We can revisit this afterwards to remove unneeded events but this looks good for now.

@gclssvglx gclssvglx merged commit 80a03a4 into main Nov 11, 2021
@gclssvglx gclssvglx deleted the remove-jquery-from-track-radio-group branch November 11, 2021 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andysellick andysellick andysellick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gclssvglx @andysellick @govuk-ci