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

GOVUK.Modules.start can accept DOM elements #2260

Merged
merged 1 commit into from
Aug 16, 2021
Merged

Conversation

kevindew
Copy link
Member

@kevindew kevindew commented Aug 11, 2021

Trello: https://trello.com/c/pATsfsuD/524-convert-finder-frontend-module-initialisers-in-livesearchjs-to-not-use-jquery

What

This adjusts GOVUK.Modules.find to be able to handle the situation where the input is not a jQuery object and can instead be a DOM element. This has been done so that GOVUK.Modules.start need not only be called with jQuery wrapped elements.

Why

The reason for this is to support GOV.UK in its journey away from jQuery. Previously we needed to add jQuery back into code so that this function could be called, whereas this change allows a regular DOM element to be injected.

It will allow us to change this code

from GOVUK.modules.start($(this.$resultsWrapper)) to GOVUK.modules.start(this.$resultsWrapper).

In the longer term I expect that we could deprecate calling this function with a jQuery object and then remove it (or replace start with an init method to match modules).

Visual Changes

None

@bevanloon bevanloon had a problem deploying to govuk-publis-module-sta-kk7tjj August 11, 2021 11:10 Failure
@kevindew kevindew force-pushed the module-start-no-jquery branch from c41e119 to 2d1efbf Compare August 11, 2021 11:12
@bevanloon bevanloon temporarily deployed to govuk-publis-module-sta-kk7tjj August 11, 2021 11:12 Inactive
@kevindew kevindew marked this pull request as ready for review August 16, 2021 09:06
Copy link
Contributor

@JonathanHallam JonathanHallam left a comment

Choose a reason for hiding this comment

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

Nicely done, thank you!

This adjusts GOVUK.Modules.find to be able to handle the situation where
the input is not a jQuery object and can instead be a DOM element. This
has been done so that GOVUK.Modules.start need not only be called with
jQuery wrapped elements.

The reason for this is to support GOV.UK in its journey away from
jQuery. Previously we needed to add jQuery back into code so that this
function could be called [1], whereas this change allows a regular DOM
element to be injected.

In the longer term I expect that we could deprecate calling this
function with a jQuery object and then remove it (or replace `start`
with an `init` method to match modules).

[1]: https://github.com/alphagov/finder-frontend/blob/67f01543cb9ce5d5331f423061630acc6619d3c0/app/assets/javascripts/live_search.js#L198
@kevindew kevindew force-pushed the module-start-no-jquery branch from 2d1efbf to 9734b48 Compare August 16, 2021 10:38
@bevanloon bevanloon temporarily deployed to govuk-publis-module-sta-ly103l August 16, 2021 10:38 Inactive
@kevindew kevindew merged commit 78ce837 into master Aug 16, 2021
@kevindew kevindew deleted the module-start-no-jquery branch August 16, 2021 13:10
kevindew added a commit to alphagov/finder-frontend that referenced this pull request Aug 16, 2021
This change removes jQuery from this script, it is dependent on
govuk_publishing_components having released alphagov/govuk_publishing_components#2260
chris-gds pushed a commit that referenced this pull request Aug 17, 2021
Includes:

* Add lazy loading to image card ([#2270](#2270))
* GOVUK.Modules.start can accept DOM elements ([#2260](#2260))
@chris-gds chris-gds mentioned this pull request Aug 17, 2021
kevindew added a commit to alphagov/finder-frontend that referenced this pull request Aug 18, 2021
This change removes jQuery from this script, it is dependent on
govuk_publishing_components having released alphagov/govuk_publishing_components#2260
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.

3 participants