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

Convert set ga code to our module format #2276

Merged
merged 3 commits into from
Nov 11, 2021
Merged

Conversation

andysellick
Copy link
Contributor

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

What

Converts the set ga id on form code to use our standard JS modules format. This is used on https://www.gov.uk/check-income-tax-current-year/sign-in/prove-identity

Why

We're trying to standardise our approach to JS and having it in our modules format means we also don't need to manually initialise this code in application.js anymore.

Visual changes

None.

Trello card: https://trello.com/c/bqdisKbm/277-from-jquery-from-government-frontend-set-ga-client-id-on-formjs-and-applicationjs-moderate-js-knowledge-required

- uses our standard module pattern for this code
- means we can remove the manual initialisation in application.js
- and add the module name to the form where the code is used
@govuk-ci govuk-ci temporarily deployed to government-f-convert-se-tsyutq November 10, 2021 12:11 Inactive
@govuk-ci govuk-ci temporarily deployed to government-f-convert-se-tsyutq November 10, 2021 12:14 Inactive
@andysellick andysellick requested a review from kevindew November 10, 2021 12:14
@govuk-ci govuk-ci temporarily deployed to government-f-convert-se-tsyutq November 10, 2021 12:35 Inactive
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks great Andy ⭐ - just a couple of small comments.

spec/javascripts/set-ga-client-id-on-form.spec.js Outdated Show resolved Hide resolved
app/assets/javascripts/application.js Show resolved Hide resolved
- no need for linting rule anymore
- class for initialisation isn't used anymore either
@andysellick andysellick force-pushed the convert-set-ga-js-to-module branch from 31a6102 to e028b8d Compare November 10, 2021 16:42
@govuk-ci govuk-ci temporarily deployed to government-f-convert-se-tsyutq November 10, 2021 16:43 Inactive
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

🕺

@andysellick andysellick merged commit 823277f into main Nov 11, 2021
@andysellick andysellick deleted the convert-set-ga-js-to-module branch November 11, 2021 07:51
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