Govuk js style #5855

jrmhaig · 2023-07-07T17:13:05Z

What

Trying to rewrite some of the Javascript following the GovUK Javascript style guide.

Ticket

N/A

Why

Some motivation is written in docs/javascript_restructure.md

In summary, the Javascript used in CCCD is written in inconsistent styles and have dependencies that are starting to make it difficult to keep the application up to date.

How

There are many javascript modules and functions and a safe approach is to refactor these one at a time. The notes in docs/javascript_restructure.md make it clear that the migration is in progress and so it is possible that a particular javascript file may exist in the old or the new format but it is understood which is to be considered 'correct'.

In this PR, two modules have been rewritten and the commit can be split into two PRs to be reviewed and merged separately, if required.

Determination calculator.

Old module: app/webpack/javascripts/modules/case_worker/claims/DeterminationCalculator.js
New class: app/webpack/javascripts/modules/determination.js

Commits: d9d5957, 0d390fd, 17f0f0e

The determination calculator is used on the claim summary page for caseworkers and calculates the assessment total based on the individual line items.

Select all checkbox

Old module: app/webpack/javascripts/modules/Modules.SelectAll.js
New class: app/webpack/javascripts/modules/selectAll.mjs

Commits: e9c4f16

The select all checkbox appears on the re-allocation page for admin case workers and allows all claims on the page to be selected or de-selected with one click. Currently (before this PR) there was a 'select all' hyperlink instead of a checkbox.

Before

After

The allocation page also has a select all checkbox but it is implemented separately and is already styled as a checkbox so this PR will make the pages more consistent. It is possible that the new SelectAll class could be used on that page too.

From the allocation page

sonarqubecloud · 2023-07-28T15:32:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

mpw5

To my eye this looks really good - the code is much more readable and it gives us a pattern to update the rest of the javascript.

I've left a couple of comments about comments, nothing major.

I definitely think it would be worth getting a front-end dev to review it too though 😁

mpw5 · 2023-10-12T11:21:21Z

app/webpack/javascripts/modules/determination.mjs

+      this.$module = $module
+    }
+
+    // Code goes here


Does something need to be added here?

mpw5 · 2023-10-12T11:23:57Z

app/webpack/javascripts/modules/determination_spec.mjs

+
+    determination = new Determination(table)
+    determination.init()
+    // console.log(determination)


mpw5 · 2023-10-12T11:24:15Z

app/webpack/javascripts/modules/determination.mjs

+      return
+    }
+
+    // Code goes here


Can this be removed now?

imtiazAhmd · 2023-10-13T14:55:12Z

app/webpack/javascripts/modules/determination.mjs

+      return 0.0
+    }
+  }
+


If you want to keep this function, then you can also try this:

function parsedValue(field) { const value = parseFloat(field?.value) || 0.0; return Math.max(0.0, value); }

imtiazAhmd · 2023-10-13T15:01:13Z

app/webpack/javascripts/modules/determination.mjs

+      document.getElementById('claim_assessment_attributes_vat_amount')
+    ].filter(field => field)
+    this.fields.forEach(element => element.addEventListener('change', () => this.calculateTotalRows()))
+  }


You can try

const fieldIds = [ 'fees', 'expenses', 'disbursements', 'vat_amount' ]; this.fields = fieldIds.map(id => document.querySelector(`#claim_assessment_attributes_${id}`)) .filter(field => field); this.fields.forEach(field => field.addEventListener('change', () => this.calculateTotalRows()));

imtiazAhmd · 2023-10-13T15:10:35Z

app/webpack/javascripts/modules/determination.mjs

+      this.$totalInclVat.innerHTML = data.total_inc_vat
+    })
+  }
+


You can use async await here. Makes applyVat to return a promise that can be awaited. Also if you are using parsedFloat only here that also can be removed/ or not !

async function calculateTotalRows() { const total = this.fields.reduce((n, field) => n + (parseFloat(field?.value) || 0.0), 0).toFixed(2); const { net_amount, vat_amount, total_inc_vat } = await this.applyVat(total); this.$totalExclVat.innerHTML = net_amount; if (this.$totalVat) { this.$totalVat.innerHTML = vat_amount; } this.$totalInclVat.innerHTML = total_inc_vat; }

I tried making this into an async function and now SonarCloud is flagging a bug at the point at which it is being called: "Promise returned in function argument where a void return was expected." I remember when I tried this first I was getting similar issues and couldn't work out how to resolve it so I opted for it not being async. Is there a way to fix this?

Try adding return Promise.resolve();

imtiazAhmd · 2023-10-13T15:17:57Z

app/webpack/javascripts/modules/determination.mjs

+      net_amount: netAmount
+    })
+    const response = await fetch(this.vatUrl + '?' + params.toString())
+


Not a suggestion but for reference, we can also remove string concatenation by using backtick.

const response = await fetch(`${this.vatUrl}?${queryString}`);

imtiazAhmd · 2023-10-13T15:22:05Z

app/webpack/javascripts/modules/determination.mjs

+
+    const $module = this.$module
+
+    this.$totalExclVat = document.getElementsByClassName('js-total-exc-vat-determination')[0]


Also:

this.$totalExclVat = this.$module.querySelector('.js-total-exc-vat-determination');

imtiazAhmd · 2023-10-13T15:27:20Z

app/webpack/javascripts/modules/selectAll_spec.mjs

+      unselectedBox.name = 'box2'
+      unselectedBox.checked = false
+      testArea.appendChild(unselectedBox)
+


For above codes, you can consider creating a function that creates checkbox.

const createCheckbox = (className, checked = false, name = '') => { const checkbox = document.createElement('input'); checkbox.type = 'checkbox'; checkbox.classList.add(className); checkbox.name = name; checkbox.checked = checked; testArea.appendChild(checkbox); return checkbox; };

Then calling it like this

unselectedBox = createCheckbox('pick-me', false, 'box2');

imtiazAhmd · 2023-10-13T15:34:05Z

app/webpack/javascripts/modules/selectAll.mjs

+    const selector = this.$module.dataset.selectAllClass
+    const collection = this.$module.dataset.collectionClass
+
+    this.selectAllBox = this.$module.getElementsByClassName(selector)[0]


querySelector are more precise.

this.selectAllBox = this.$module.querySelector(`.${selector}`);

imtiazAhmd · 2023-10-13T15:34:58Z

app/webpack/javascripts/modules/selectAll.mjs

+
+    this.selectAllBox = this.$module.getElementsByClassName(selector)[0]
+    this.selectAllBox.addEventListener('change', () => this.toggleSelection())
+    this.checkBoxes = document.getElementsByClassName(collection)


Similarly

this.checkBoxes = document.querySelectorAll(`.${collection}`);

imtiazAhmd · 2023-10-13T15:41:18Z

app/webpack/javascripts/modules/selectAll.mjs

+    if (!this.$module) {
+      return
+    }
+


Can be one liner

if (!this.$module) return

... and without jQuery.

For some reason Response is not being recognised even though it is part of Javascript (https://caniuse.com/?search=Response). This is possibly due to out of date dependencies preventing ES6 coding standards being checkd. It is added to the list of globals to allow `yarn standard` to pass. It may be possible to remove this after refactoring.

sonarqubecloud · 2023-10-30T08:46:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
32.8% Duplication

jrmhaig force-pushed the govuk_js_style branch 2 times, most recently from 4f737d0 to 0050eec Compare July 10, 2023 08:29

jrmhaig added the WIP label Jul 10, 2023

jrmhaig force-pushed the govuk_js_style branch 3 times, most recently from f66f596 to 7c9ed21 Compare July 10, 2023 08:48

jrmhaig force-pushed the govuk_js_style branch 7 times, most recently from 7e90b66 to fdcc302 Compare July 28, 2023 15:32

jrmhaig force-pushed the govuk_js_style branch from fdcc302 to 07b6042 Compare September 22, 2023 08:16

jrmhaig force-pushed the govuk_js_style branch 5 times, most recently from 3002588 to b760016 Compare October 11, 2023 11:45

jrmhaig removed the WIP label Oct 11, 2023

jrmhaig marked this pull request as ready for review October 11, 2023 13:14

jrmhaig requested review from a team as code owners October 11, 2023 13:14

mpw5 reviewed Oct 12, 2023

View reviewed changes

imtiazAhmd reviewed Oct 13, 2023

View reviewed changes

app/webpack/javascripts/modules/selectAll.mjs

if (!this.$module) {

return

}

Copy link

Contributor

imtiazAhmd Oct 13, 2023

Choose a reason for hiding this comment

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

Can be one liner

if (!this.$module) return

jrmhaig force-pushed the govuk_js_style branch 2 times, most recently from 0f7c885 to 4d33c4b Compare October 20, 2023 09:53

Write motivation

0e0ca9b

jrmhaig force-pushed the govuk_js_style branch 6 times, most recently from 9486330 to 99b6e79 Compare October 20, 2023 18:00

jrmhaig added 2 commits October 20, 2023 19:00

Determination javascript using Gov.UK recommended style

5629cd9

... and without jQuery.

Re-write 'select all'

1f3813f

jrmhaig force-pushed the govuk_js_style branch 3 times, most recently from 4b05925 to 9719762 Compare October 27, 2023 11:49

jrmhaig force-pushed the govuk_js_style branch from 9719762 to ea44184 Compare October 30, 2023 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Govuk js style #5855

Govuk js style #5855

jrmhaig commented Jul 7, 2023 •

edited

Loading

sonarqubecloud bot commented Jul 28, 2023

mpw5 left a comment

mpw5 Oct 12, 2023

mpw5 Oct 12, 2023

mpw5 Oct 12, 2023

imtiazAhmd Oct 13, 2023 •

edited

Loading

imtiazAhmd Oct 13, 2023 •

edited

Loading

imtiazAhmd Oct 13, 2023

jrmhaig Oct 18, 2023

imtiazAhmd Oct 20, 2023

imtiazAhmd Oct 13, 2023

imtiazAhmd Oct 13, 2023

imtiazAhmd Oct 13, 2023

imtiazAhmd Oct 13, 2023

imtiazAhmd Oct 13, 2023

imtiazAhmd Oct 13, 2023

sonarqubecloud bot commented Oct 30, 2023


		const $module = this.$module

		this.$totalExclVat = document.getElementsByClassName('js-total-exc-vat-determination')[0]

Govuk js style #5855

Are you sure you want to change the base?

Govuk js style #5855

Conversation

jrmhaig commented Jul 7, 2023 • edited Loading

What

Ticket

Why

How

Determination calculator.

Select all checkbox

Before

After

From the allocation page

sonarqubecloud bot commented Jul 28, 2023

mpw5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtiazAhmd Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

imtiazAhmd Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 30, 2023

jrmhaig commented Jul 7, 2023 •

edited

Loading

imtiazAhmd Oct 13, 2023 •

edited

Loading

imtiazAhmd Oct 13, 2023 •

edited

Loading