From 27deff0c7bd9c1461d56b61a5b1abcee14fac439 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Tue, 22 Nov 2022 15:38:11 +0000 Subject: [PATCH 01/15] =?UTF-8?q?Fix=20compiler=20=E2=80=9CThis=20expressi?= =?UTF-8?q?on=20is=20not=20callable=E2=80=9D=20for=20`outdent`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/jest-helpers.js | 2 +- src/govuk/helpers/grid.test.js | 2 +- src/govuk/helpers/spacing.test.js | 2 +- src/govuk/helpers/typography.test.js | 2 +- src/govuk/objects/width-container.test.js | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/jest-helpers.js b/lib/jest-helpers.js index 216ac426fd..c1c9f6c0eb 100644 --- a/lib/jest-helpers.js +++ b/lib/jest-helpers.js @@ -5,7 +5,7 @@ const cheerio = require('cheerio') const { configureAxe } = require('jest-axe') const sass = require('node-sass') const nunjucks = require('nunjucks') -const outdent = require('outdent') +const { outdent } = require('outdent') const sassRender = util.promisify(sass.render) diff --git a/src/govuk/helpers/grid.test.js b/src/govuk/helpers/grid.test.js index dd0bf999c4..f346fb118d 100644 --- a/src/govuk/helpers/grid.test.js +++ b/src/govuk/helpers/grid.test.js @@ -1,4 +1,4 @@ -const outdent = require('outdent') +const { outdent } = require('outdent') const { renderSass } = require('../../../lib/jest-helpers') diff --git a/src/govuk/helpers/spacing.test.js b/src/govuk/helpers/spacing.test.js index 879c05ec79..4671f478ff 100644 --- a/src/govuk/helpers/spacing.test.js +++ b/src/govuk/helpers/spacing.test.js @@ -1,4 +1,4 @@ -const outdent = require('outdent') +const { outdent } = require('outdent') const { renderSass } = require('../../../lib/jest-helpers') diff --git a/src/govuk/helpers/typography.test.js b/src/govuk/helpers/typography.test.js index 236beb4a68..02e5a20540 100644 --- a/src/govuk/helpers/typography.test.js +++ b/src/govuk/helpers/typography.test.js @@ -1,6 +1,6 @@ const sass = require('node-sass') -const outdent = require('outdent') +const { outdent } = require('outdent') const { renderSass } = require('../../../lib/jest-helpers') diff --git a/src/govuk/objects/width-container.test.js b/src/govuk/objects/width-container.test.js index f0e5ff15bd..ac3203d8a4 100644 --- a/src/govuk/objects/width-container.test.js +++ b/src/govuk/objects/width-container.test.js @@ -1,4 +1,4 @@ -const outdent = require('outdent') +const { outdent } = require('outdent') const { renderSass } = require('../../../lib/jest-helpers') From 5f3cc7a583c9edc7f9d5d0d7140932bd7edaf8f6 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 9 Nov 2022 14:12:22 +0000 Subject: [PATCH 02/15] =?UTF-8?q?Fix=20compiler=20=E2=80=9CExpected=201=20?= =?UTF-8?q?arguments,=20but=20got=202=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/govuk/components/tabs/tabs.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/govuk/components/tabs/tabs.mjs b/src/govuk/components/tabs/tabs.mjs index 56b625d6b4..4aa9f3af90 100644 --- a/src/govuk/components/tabs/tabs.mjs +++ b/src/govuk/components/tabs/tabs.mjs @@ -112,7 +112,7 @@ Tabs.prototype.teardown = function () { $tabList.removeAttribute('role') nodeListForEach($tabListItems, function ($item) { - $item.removeAttribute('role', 'presentation') + $item.removeAttribute('role') }) nodeListForEach($tabs, function ($tab) { From 503e04fd9ed6c3b1551ad4389671202e85bc614b Mon Sep 17 00:00:00 2001 From: Romaric Date: Tue, 13 Dec 2022 12:32:48 +0000 Subject: [PATCH 03/15] Use generics for typing `nodeListForEach` This allows to forward the type of the `NodeListOf` it iterates on to the parameters of the callback. In turn this should allow to drop quite a few `instanceof` checks when using the function by properly typing the `NodeList` we give it. Interesting links: - [The `@template` JSDoc tag](https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#template) - [How to constrain the generic type](https://stackoverflow.com/a/54631901) --- src/govuk/common/index.mjs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/govuk/common/index.mjs b/src/govuk/common/index.mjs index 1b54b08aeb..d798fb5c67 100644 --- a/src/govuk/common/index.mjs +++ b/src/govuk/common/index.mjs @@ -13,8 +13,9 @@ * This seems to fail in IE8, requires more investigation. * See: https://github.com/imagitama/nodelist-foreach-polyfill * - * @param {NodeListOf} nodes - NodeList from querySelectorAll() - * @param {nodeListIterator} callback - Callback function to run for each node + * @template {Node} ElementType + * @param {NodeListOf} nodes - NodeList from querySelectorAll() + * @param {nodeListIterator} callback - Callback function to run for each node * @returns {void} */ export function nodeListForEach (nodes, callback) { @@ -158,9 +159,10 @@ export function extractConfigByNamespace (configObject, namespace) { } /** + * @template {Node} ElementType * @callback nodeListIterator - * @param {Element} value - The current node being iterated on + * @param {ElementType} value - The current node being iterated on * @param {number} index - The current index in the iteration - * @param {NodeListOf} nodes - NodeList from querySelectorAll() + * @param {NodeListOf} nodes - NodeList from querySelectorAll() * @returns {void} */ From d68b694c1a307a0ca292ca89a3299e1d6ee10c00 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 19 Jan 2023 13:33:27 +0000 Subject: [PATCH 04/15] =?UTF-8?q?Fix=20compiler=20=E2=80=9CProperty=20'xxx?= =?UTF-8?q?x'=20does=20not=20exist=20on=20type=20'xxxx'=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Various `instanceof` additions to help the `tsc` compiler discover potential issues For example a mouse or keyboard `event.target` might not necessarily be the element we expect --- src/govuk/all.mjs | 4 ++- src/govuk/components/accordion/accordion.mjs | 32 +++++++++++-------- .../character-count/character-count.mjs | 15 +++++---- .../components/checkboxes/checkboxes.mjs | 18 +++++++---- .../error-summary/error-summary.mjs | 2 +- src/govuk/components/radios/radios.mjs | 14 +++++--- src/govuk/components/skip-link/skip-link.mjs | 2 +- src/govuk/components/tabs/tabs.mjs | 20 ++++++------ src/govuk/components/tabs/tabs.test.js | 2 +- src/govuk/i18n.mjs | 6 ++-- 10 files changed, 68 insertions(+), 47 deletions(-) diff --git a/src/govuk/all.mjs b/src/govuk/all.mjs index ce7e42e930..f90ce7c139 100644 --- a/src/govuk/all.mjs +++ b/src/govuk/all.mjs @@ -75,7 +75,9 @@ function initAll (config) { // Find first skip link module to enhance. var $skipLink = $scope.querySelector('[data-module="govuk-skip-link"]') - new SkipLink($skipLink).init() + if ($skipLink) { + new SkipLink($skipLink).init() + } var $tabs = $scope.querySelectorAll('[data-module="govuk-tabs"]') nodeListForEach($tabs, function ($tabs) { diff --git a/src/govuk/components/accordion/accordion.mjs b/src/govuk/components/accordion/accordion.mjs index aea9a6cf4c..58fc18848b 100644 --- a/src/govuk/components/accordion/accordion.mjs +++ b/src/govuk/components/accordion/accordion.mjs @@ -139,26 +139,30 @@ Accordion.prototype.initControls = function () { * Initialise section headers */ Accordion.prototype.initSectionHeaders = function () { - // Loop through section headers - nodeListForEach(this.$sections, function ($section, i) { + var $component = this + var $sections = this.$sections + + // Loop through sections + nodeListForEach($sections, function ($section, i) { + var $header = $section.querySelector('.' + $component.sectionHeaderClass) + // Set header attributes - var $header = $section.querySelector('.' + this.sectionHeaderClass) - this.constructHeaderMarkup($header, i) - this.setExpanded(this.isExpanded($section), $section) + $component.constructHeaderMarkup($header, i) + $component.setExpanded($component.isExpanded($section), $section) // Handle events - $header.addEventListener('click', this.onSectionToggle.bind(this, $section)) + $header.addEventListener('click', $component.onSectionToggle.bind($component, $section)) // See if there is any state stored in sessionStorage and set the sections to // open or closed. - this.setInitialState($section) - }.bind(this)) + $component.setInitialState($section) + }) } /** * Construct section header * - * @param {HTMLDivElement} $header - Section header + * @param {HTMLElement} $header - Section header * @param {number} index - Section index */ Accordion.prototype.constructHeaderMarkup = function ($header, index) { @@ -286,17 +290,19 @@ Accordion.prototype.onSectionToggle = function ($section) { * When Open/Close All toggled, set and store state */ Accordion.prototype.onShowOrHideAllToggle = function () { - var $module = this + var $component = this var $sections = this.$sections + var nowExpanded = !this.checkIfAllSectionsOpen() + // Loop through sections nodeListForEach($sections, function ($section) { - $module.setExpanded(nowExpanded, $section) + $component.setExpanded(nowExpanded, $section) // Store the state in sessionStorage when a change is triggered - $module.storeState($section) + $component.storeState($section) }) - $module.updateShowAllButton(nowExpanded) + $component.updateShowAllButton(nowExpanded) } /** diff --git a/src/govuk/components/character-count/character-count.mjs b/src/govuk/components/character-count/character-count.mjs index 8b46790df6..ed8b8901af 100644 --- a/src/govuk/components/character-count/character-count.mjs +++ b/src/govuk/components/character-count/character-count.mjs @@ -96,9 +96,9 @@ function CharacterCount ($module, config) { }) // Determine the limit attribute (characters or words) - if (this.config.maxwords) { + if ('maxwords' in this.config && this.config.maxwords) { this.maxLength = this.config.maxwords - } else if (this.config.maxlength) { + } else if ('maxlength' in this.config && this.config.maxlength) { this.maxLength = this.config.maxlength } else { return @@ -108,7 +108,9 @@ function CharacterCount ($module, config) { this.$textarea = $module.querySelector('.govuk-js-character-count') this.$visibleCountMessage = null this.$screenReaderCountMessage = null + this.lastInputTimestamp = null + this.lastInputValue = '' } /** @@ -233,9 +235,8 @@ CharacterCount.prototype.handleBlur = function () { * Update count message if textarea value has changed */ CharacterCount.prototype.updateIfValueChanged = function () { - if (!this.$textarea.oldValue) this.$textarea.oldValue = '' - if (this.$textarea.value !== this.$textarea.oldValue) { - this.$textarea.oldValue = this.$textarea.value + if (this.$textarea.value !== this.lastInputValue) { + this.lastInputValue = this.$textarea.value this.updateCountMessage() } } @@ -308,7 +309,7 @@ CharacterCount.prototype.updateScreenReaderCountMessage = function () { * @returns {number} the number of characters (or words) in the text */ CharacterCount.prototype.count = function (text) { - if (this.config.maxwords) { + if ('maxwords' in this.config && this.config.maxwords) { var tokens = text.match(/\S+/g) || [] // Matches consecutive non-whitespace chars return tokens.length } else { @@ -324,7 +325,7 @@ CharacterCount.prototype.count = function (text) { CharacterCount.prototype.getCountMessage = function () { var remainingNumber = this.maxLength - this.count(this.$textarea.value) - var countType = this.config.maxwords ? 'words' : 'characters' + var countType = 'maxwords' in this.config && this.config.maxwords ? 'words' : 'characters' return this.formatCountMessage(remainingNumber, countType) } diff --git a/src/govuk/components/checkboxes/checkboxes.mjs b/src/govuk/components/checkboxes/checkboxes.mjs index 480cb4f077..724255515e 100644 --- a/src/govuk/components/checkboxes/checkboxes.mjs +++ b/src/govuk/components/checkboxes/checkboxes.mjs @@ -103,15 +103,19 @@ Checkboxes.prototype.syncConditionalRevealWithInputState = function ($input) { * @param {HTMLElement} $input - Checkbox input */ Checkboxes.prototype.unCheckAllInputsExcept = function ($input) { - var allInputsWithSameName = document.querySelectorAll('input[type="checkbox"][name="' + $input.name + '"]') + var $component = this + + var allInputsWithSameName = document.querySelectorAll( + 'input[type="checkbox"][name="' + $input.name + '"]' + ) nodeListForEach(allInputsWithSameName, function ($inputWithSameName) { var hasSameFormOwner = ($input.form === $inputWithSameName.form) if (hasSameFormOwner && $inputWithSameName !== $input) { $inputWithSameName.checked = false - this.syncConditionalRevealWithInputState($inputWithSameName) + $component.syncConditionalRevealWithInputState($inputWithSameName) } - }.bind(this)) + }) } /** @@ -124,6 +128,8 @@ Checkboxes.prototype.unCheckAllInputsExcept = function ($input) { * @param {HTMLInputElement} $input - Checkbox input */ Checkboxes.prototype.unCheckExclusiveInputs = function ($input) { + var $component = this + var allInputsWithSameNameAndExclusiveBehaviour = document.querySelectorAll( 'input[data-behaviour="exclusive"][type="checkbox"][name="' + $input.name + '"]' ) @@ -132,9 +138,9 @@ Checkboxes.prototype.unCheckExclusiveInputs = function ($input) { var hasSameFormOwner = ($input.form === $exclusiveInput.form) if (hasSameFormOwner) { $exclusiveInput.checked = false - this.syncConditionalRevealWithInputState($exclusiveInput) + $component.syncConditionalRevealWithInputState($exclusiveInput) } - }.bind(this)) + }) } /** @@ -149,7 +155,7 @@ Checkboxes.prototype.handleClick = function (event) { var $clickedInput = event.target // Ignore clicks on things that aren't checkbox inputs - if ($clickedInput.type !== 'checkbox') { + if (!($clickedInput instanceof HTMLInputElement) || $clickedInput.type !== 'checkbox') { return } diff --git a/src/govuk/components/error-summary/error-summary.mjs b/src/govuk/components/error-summary/error-summary.mjs index 1154cf1107..7cbb785fb1 100644 --- a/src/govuk/components/error-summary/error-summary.mjs +++ b/src/govuk/components/error-summary/error-summary.mjs @@ -107,7 +107,7 @@ ErrorSummary.prototype.handleClick = function (event) { */ ErrorSummary.prototype.focusTarget = function ($target) { // If the element that was clicked was not a link, return early - if ($target.tagName !== 'A' || $target.href === false) { + if (!($target instanceof HTMLAnchorElement)) { return false } diff --git a/src/govuk/components/radios/radios.mjs b/src/govuk/components/radios/radios.mjs index eb2b16f6fe..0fbc3d8ca3 100644 --- a/src/govuk/components/radios/radios.mjs +++ b/src/govuk/components/radios/radios.mjs @@ -105,10 +105,11 @@ Radios.prototype.syncConditionalRevealWithInputState = function ($input) { * @param {MouseEvent} event - Click event */ Radios.prototype.handleClick = function (event) { + var $component = this var $clickedInput = event.target // Ignore clicks on things that aren't radio buttons - if ($clickedInput.type !== 'radio') { + if (!($clickedInput instanceof HTMLInputElement) || $clickedInput.type !== 'radio') { return } @@ -116,14 +117,17 @@ Radios.prototype.handleClick = function (event) { // aria-controls attributes. var $allInputs = document.querySelectorAll('input[type="radio"][aria-controls]') + var $clickedInputForm = $clickedInput.form + var $clickedInputName = $clickedInput.name + nodeListForEach($allInputs, function ($input) { - var hasSameFormOwner = ($input.form === $clickedInput.form) - var hasSameName = ($input.name === $clickedInput.name) + var hasSameFormOwner = $input.form === $clickedInputForm + var hasSameName = $input.name === $clickedInputName if (hasSameName && hasSameFormOwner) { - this.syncConditionalRevealWithInputState($input) + $component.syncConditionalRevealWithInputState($input) } - }.bind(this)) + }) } export default Radios diff --git a/src/govuk/components/skip-link/skip-link.mjs b/src/govuk/components/skip-link/skip-link.mjs index 5b930a03d1..29401c0c93 100644 --- a/src/govuk/components/skip-link/skip-link.mjs +++ b/src/govuk/components/skip-link/skip-link.mjs @@ -8,7 +8,7 @@ import '../../vendor/polyfills/Function/prototype/bind.mjs' * Skip link component * * @class - * @param {HTMLElement} $module - HTML element to use for skip link + * @param {HTMLAnchorElement} $module - HTML element to use for skip link */ function SkipLink ($module) { this.$module = $module diff --git a/src/govuk/components/tabs/tabs.mjs b/src/govuk/components/tabs/tabs.mjs index 4aa9f3af90..1e08f7e478 100644 --- a/src/govuk/components/tabs/tabs.mjs +++ b/src/govuk/components/tabs/tabs.mjs @@ -56,6 +56,7 @@ Tabs.prototype.checkMode = function () { * Setup tab component */ Tabs.prototype.setup = function () { + var $component = this var $module = this.$module var $tabs = this.$tabs var $tabList = $module.querySelector('.govuk-tabs__list') @@ -73,19 +74,19 @@ Tabs.prototype.setup = function () { nodeListForEach($tabs, function ($tab) { // Set HTML attributes - this.setAttributes($tab) + $component.setAttributes($tab) // Save bounded functions to use when removing event listeners during teardown - $tab.boundTabClick = this.onTabClick.bind(this) - $tab.boundTabKeydown = this.onTabKeydown.bind(this) + $tab.boundTabClick = $component.onTabClick.bind($component) + $tab.boundTabKeydown = $component.onTabKeydown.bind($component) // Handle events $tab.addEventListener('click', $tab.boundTabClick, true) $tab.addEventListener('keydown', $tab.boundTabKeydown, true) // Remove old active panels - this.hideTab($tab) - }.bind(this)) + $component.hideTab($tab) + }) // Show either the active tab according to the URL's hash or the first tab var $activeTab = this.getTab(window.location.hash) || this.$tabs[0] @@ -100,6 +101,7 @@ Tabs.prototype.setup = function () { * Teardown tab component */ Tabs.prototype.teardown = function () { + var $component = this var $module = this.$module var $tabs = this.$tabs var $tabList = $module.querySelector('.govuk-tabs__list') @@ -121,8 +123,8 @@ Tabs.prototype.teardown = function () { $tab.removeEventListener('keydown', $tab.boundTabKeydown, true) // Unset HTML attributes - this.unsetAttributes($tab) - }.bind(this)) + $component.unsetAttributes($tab) + }) // Remove hashchange event handler window.removeEventListener('hashchange', $module.boundOnHashChange, true) @@ -372,7 +374,7 @@ Tabs.prototype.hidePanel = function ($tab) { */ Tabs.prototype.unhighlightTab = function ($tab) { $tab.setAttribute('aria-selected', 'false') - $tab.parentNode.classList.remove('govuk-tabs__list-item--selected') + $tab.parentElement.classList.remove('govuk-tabs__list-item--selected') $tab.setAttribute('tabindex', '-1') } @@ -383,7 +385,7 @@ Tabs.prototype.unhighlightTab = function ($tab) { */ Tabs.prototype.highlightTab = function ($tab) { $tab.setAttribute('aria-selected', 'true') - $tab.parentNode.classList.add('govuk-tabs__list-item--selected') + $tab.parentElement.classList.add('govuk-tabs__list-item--selected') $tab.setAttribute('tabindex', '0') } diff --git a/src/govuk/components/tabs/tabs.test.js b/src/govuk/components/tabs/tabs.test.js index 22c56a718b..2e478e6e75 100644 --- a/src/govuk/components/tabs/tabs.test.js +++ b/src/govuk/components/tabs/tabs.test.js @@ -133,7 +133,7 @@ describe('/components/tabs', () => { const currentTabAriaSelected = await page.evaluate(() => document.body.querySelector('.govuk-tabs__tab[href="#past-week"]').getAttribute('aria-selected')) expect(currentTabAriaSelected).toEqual('true') - const currentTabClasses = await page.evaluate(() => document.body.querySelector('.govuk-tabs__tab[href="#past-week"]').parentNode.className) + const currentTabClasses = await page.evaluate(() => document.body.querySelector('.govuk-tabs__tab[href="#past-week"]').parentElement.className) expect(currentTabClasses).toContain('govuk-tabs__list-item--selected') const currentTabPanelIsHidden = await page.evaluate(() => document.getElementById('past-week').classList.contains('govuk-tabs__panel--hidden')) diff --git a/src/govuk/i18n.mjs b/src/govuk/i18n.mjs index 50fd25c62c..eb88e1c326 100644 --- a/src/govuk/i18n.mjs +++ b/src/govuk/i18n.mjs @@ -40,10 +40,10 @@ I18n.prototype.t = function (lookupKey, options) { lookupKey = lookupKey + '.' + this.getPluralSuffix(lookupKey, options.count) } - if (lookupKey in this.translations) { - // Fetch the translation string for that lookup key - var translationString = this.translations[lookupKey] + // Fetch the translation string for that lookup key + var translationString = this.translations[lookupKey] + if (typeof translationString === 'string') { // Check for ${} placeholders in the translation string if (translationString.match(/%{(.\S+)}/)) { if (!options) { From d48170062016e559cf905644ed5b8eb4a1e65cb7 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Mon, 19 Dec 2022 12:47:28 +0000 Subject: [PATCH 05/15] =?UTF-8?q?Fix=20compiler=20=E2=80=9CType=20'boolean?= =?UTF-8?q?'=20is=20not=20assignable=20to=20type=20'string'=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/govuk/components/accordion/accordion.mjs | 4 ++-- src/govuk/components/character-count/character-count.mjs | 2 +- src/govuk/components/checkboxes/checkboxes.mjs | 2 +- src/govuk/components/header/header.mjs | 2 +- src/govuk/components/radios/radios.mjs | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/govuk/components/accordion/accordion.mjs b/src/govuk/components/accordion/accordion.mjs index 58fc18848b..c94627e634 100644 --- a/src/govuk/components/accordion/accordion.mjs +++ b/src/govuk/components/accordion/accordion.mjs @@ -322,7 +322,7 @@ Accordion.prototype.setExpanded = function (expanded, $section) { : this.i18n.t('showSection') $showHideText.innerText = newButtonText - $button.setAttribute('aria-expanded', expanded) + $button.setAttribute('aria-expanded', expanded.toString()) // Update aria-label combining var ariaLabelParts = [] @@ -400,7 +400,7 @@ Accordion.prototype.updateShowAllButton = function (expanded) { ? this.i18n.t('hideAllSections') : this.i18n.t('showAllSections') - this.$showAllButton.setAttribute('aria-expanded', expanded) + this.$showAllButton.setAttribute('aria-expanded', expanded.toString()) this.$showAllText.innerText = newButtonText // Swap icon, toggle class diff --git a/src/govuk/components/character-count/character-count.mjs b/src/govuk/components/character-count/character-count.mjs index ed8b8901af..8c0f3cc3ec 100644 --- a/src/govuk/components/character-count/character-count.mjs +++ b/src/govuk/components/character-count/character-count.mjs @@ -294,7 +294,7 @@ CharacterCount.prototype.updateScreenReaderCountMessage = function () { if (this.isOverThreshold()) { $screenReaderCountMessage.removeAttribute('aria-hidden') } else { - $screenReaderCountMessage.setAttribute('aria-hidden', true) + $screenReaderCountMessage.setAttribute('aria-hidden', 'true') } // Update message diff --git a/src/govuk/components/checkboxes/checkboxes.mjs b/src/govuk/components/checkboxes/checkboxes.mjs index 724255515e..fcac5fa4b2 100644 --- a/src/govuk/components/checkboxes/checkboxes.mjs +++ b/src/govuk/components/checkboxes/checkboxes.mjs @@ -89,7 +89,7 @@ Checkboxes.prototype.syncConditionalRevealWithInputState = function ($input) { if ($target && $target.classList.contains('govuk-checkboxes__conditional')) { var inputIsChecked = $input.checked - $input.setAttribute('aria-expanded', inputIsChecked) + $input.setAttribute('aria-expanded', inputIsChecked.toString()) $target.classList.toggle('govuk-checkboxes__conditional--hidden', !inputIsChecked) } } diff --git a/src/govuk/components/header/header.mjs b/src/govuk/components/header/header.mjs index 567a796bfb..c2c1e4c161 100644 --- a/src/govuk/components/header/header.mjs +++ b/src/govuk/components/header/header.mjs @@ -77,7 +77,7 @@ Header.prototype.syncState = function () { this.$menuButton.setAttribute('hidden', '') } else { this.$menuButton.removeAttribute('hidden') - this.$menuButton.setAttribute('aria-expanded', this.menuIsOpen) + this.$menuButton.setAttribute('aria-expanded', this.menuIsOpen.toString()) if (this.menuIsOpen) { this.$menu.removeAttribute('hidden') diff --git a/src/govuk/components/radios/radios.mjs b/src/govuk/components/radios/radios.mjs index 0fbc3d8ca3..92a843e572 100644 --- a/src/govuk/components/radios/radios.mjs +++ b/src/govuk/components/radios/radios.mjs @@ -89,7 +89,7 @@ Radios.prototype.syncConditionalRevealWithInputState = function ($input) { if ($target && $target.classList.contains('govuk-radios__conditional')) { var inputIsChecked = $input.checked - $input.setAttribute('aria-expanded', inputIsChecked) + $input.setAttribute('aria-expanded', inputIsChecked.toString()) $target.classList.toggle('govuk-radios__conditional--hidden', !inputIsChecked) } } From b694c4b72ed479e07b1e52cfdfbcc32eabba8b5f Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Mon, 5 Dec 2022 14:17:53 +0000 Subject: [PATCH 06/15] =?UTF-8?q?Fix=20compiler=20=E2=80=9CType=20'number'?= =?UTF-8?q?=20is=20not=20assignable=20to=20type=20'string'=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/govuk/common/normalise-dataset.mjs | 2 +- src/govuk/i18n.mjs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/govuk/common/normalise-dataset.mjs b/src/govuk/common/normalise-dataset.mjs index 2b2f491fb8..338ee633ff 100644 --- a/src/govuk/common/normalise-dataset.mjs +++ b/src/govuk/common/normalise-dataset.mjs @@ -34,7 +34,7 @@ export function normaliseString (value) { // Empty / whitespace-only strings are considered finite so we need to check // the length of the trimmed string as well - if (trimmedValue.length > 0 && isFinite(trimmedValue)) { + if (trimmedValue.length > 0 && isFinite(Number(trimmedValue))) { return Number(trimmedValue) } diff --git a/src/govuk/i18n.mjs b/src/govuk/i18n.mjs index eb88e1c326..40050c1faa 100644 --- a/src/govuk/i18n.mjs +++ b/src/govuk/i18n.mjs @@ -33,9 +33,9 @@ I18n.prototype.t = function (lookupKey, options) { } // If the `count` option is set, determine which plural suffix is needed and - // change the lookupKey to match. We check to see if it's undefined instead of + // change the lookupKey to match. We check to see if it's numeric instead of // falsy, as this could legitimately be 0. - if (options && typeof options.count !== 'undefined') { + if (options && typeof options.count === 'number') { // Get the plural suffix lookupKey = lookupKey + '.' + this.getPluralSuffix(lookupKey, options.count) } @@ -87,8 +87,8 @@ I18n.prototype.replacePlaceholders = function (translationString, options) { } // If the placeholder's value is a number, localise the number formatting - if (typeof placeholderValue === 'number' && formatter) { - return formatter.format(placeholderValue) + if (typeof placeholderValue === 'number') { + return formatter ? formatter.format(placeholderValue) : placeholderValue.toString() } return placeholderValue From 247d086c7157477596b26f99cdc117ff0e63efd7 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Mon, 5 Dec 2022 14:05:42 +0000 Subject: [PATCH 07/15] =?UTF-8?q?Fix=20compiler=20=E2=80=9CType=20'string'?= =?UTF-8?q?=20is=20not=20assignable=20to=20type=20'PluralRuleName'?= =?UTF-8?q?=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I18n.pluralRulesMap keys are variable so “string” wasn’t compatible when the compiler thought we were using fixed string literals Might be worth another look at `Object.keys()` in future with: * https://github.com/microsoft/TypeScript/pull/45464 --- src/govuk/i18n.mjs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/govuk/i18n.mjs b/src/govuk/i18n.mjs index 40050c1faa..e398907fde 100644 --- a/src/govuk/i18n.mjs +++ b/src/govuk/i18n.mjs @@ -211,7 +211,7 @@ I18n.prototype.selectPluralFormUsingFallbackRules = function (count) { * regardless of region. There are exceptions, however, (e.g. Portuguese) so * this searches by both the full and shortened locale codes, just to be sure. * - * @returns {PluralRuleName | undefined} The name of the pluralisation rule to use (a key for one + * @returns {string | undefined} The name of the pluralisation rule to use (a key for one * of the functions in this.pluralRules) */ I18n.prototype.getPluralRulesForLocale = function () { @@ -262,7 +262,7 @@ I18n.prototype.getPluralRulesForLocale = function () { * Spanish: European Portuguese (pt-PT), Italian (it), Spanish (es) * Welsh: Welsh (cy) * - * @type {Object} + * @type {Object} */ I18n.pluralRulesMap = { arabic: ['ar'], @@ -350,12 +350,6 @@ I18n.pluralRules = { /* eslint-enable jsdoc/require-jsdoc */ } -/** - * Supported languages for plural rules - * - * @typedef {'arabic' | 'chinese' | 'french' | 'german' | 'irish' | 'russian' | 'scottish' | 'spanish' | 'welsh'} PluralRuleName - */ - /** * Plural rule category mnemonic tags * From 149b1330ffc0f40c62f3274304de2a32687eb440 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Mon, 19 Dec 2022 17:25:36 +0000 Subject: [PATCH 08/15] =?UTF-8?q?Fix=20compiler=20=E2=80=9CType=20'unknown?= =?UTF-8?q?'=20is=20not=20assignable=20to=20type=20'string'=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/govuk/i18n.mjs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/govuk/i18n.mjs b/src/govuk/i18n.mjs index e398907fde..d1318d7f80 100644 --- a/src/govuk/i18n.mjs +++ b/src/govuk/i18n.mjs @@ -82,7 +82,10 @@ I18n.prototype.replacePlaceholders = function (translationString, options) { // If a user has passed `false` as the value for the placeholder // treat it as though the value should not be displayed - if (placeholderValue === false) { + if (placeholderValue === false || ( + typeof placeholderValue !== 'number' && + typeof placeholderValue !== 'string') + ) { return '' } From d085607f72f5e0b6cfeffab4dcf0bdea82a70c46 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 11 Jan 2023 21:00:18 +0000 Subject: [PATCH 09/15] =?UTF-8?q?Fix=20ESLint=20=E2=80=9COperands=20of=20'?= =?UTF-8?q?+'=20operation=20must=20either=20be=20both=20strings=20or=20bot?= =?UTF-8?q?h=20numbers=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/govuk/components/accordion/accordion.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/govuk/components/accordion/accordion.mjs b/src/govuk/components/accordion/accordion.mjs index c94627e634..b3015bc689 100644 --- a/src/govuk/components/accordion/accordion.mjs +++ b/src/govuk/components/accordion/accordion.mjs @@ -173,7 +173,7 @@ Accordion.prototype.constructHeaderMarkup = function ($header, index) { // Create a button element that will replace the '.govuk-accordion__section-button' span var $button = document.createElement('button') $button.setAttribute('type', 'button') - $button.setAttribute('aria-controls', this.$module.id + '-content-' + (index + 1)) + $button.setAttribute('aria-controls', this.$module.id + '-content-' + (index + 1).toString()) // Copy all attributes (https://developer.mozilla.org/en-US/docs/Web/API/Element/attributes) from $span to $button for (var i = 0; i < $span.attributes.length; i++) { From be9aca78bb60dd88335b45df5f9f65f7b1cebefb Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 15 Dec 2022 17:19:24 +0000 Subject: [PATCH 10/15] Fix various param/return types in review app Including shared file and helper functions --- app/app.js | 6 ++-- lib/file-helper.js | 62 +++++++++++++++++++++++++++-------------- lib/helper-functions.js | 10 ++----- lib/jest-helpers.js | 7 ++++- 4 files changed, 53 insertions(+), 32 deletions(-) diff --git a/app/app.js b/app/app.js index c35018c2c2..5aba47df76 100644 --- a/app/app.js +++ b/app/app.js @@ -155,14 +155,14 @@ module.exports = async (options) => { }) // Component example preview - app.get('/components/:componentName/:exampleName*?/preview', function (req, res, next) { + app.get('/components/:componentName/:exampleName?/preview', function (req, res, next) { // Find the data for the specified example (or the default example) const componentName = req.params.componentName const exampleName = req.params.exampleName || 'default' - const previewLayout = res.locals.componentData.previewLayout || 'layout' + const previewLayout = res.locals.componentData?.previewLayout || 'layout' - const exampleConfig = res.locals.componentData.examples.find( + const exampleConfig = res.locals.componentData?.examples.find( example => example.name.replace(/ /g, '-') === exampleName ) diff --git a/lib/file-helper.js b/lib/file-helper.js index d9970b8331..3ebcfcb40a 100644 --- a/lib/file-helper.js +++ b/lib/file-helper.js @@ -1,5 +1,5 @@ const { readFile } = require('fs/promises') -const { join, parse, ParsedPath, relative } = require('path') +const { join, parse, relative } = require('path') const { promisify } = require('util') const glob = promisify(require('glob')) @@ -14,6 +14,11 @@ const configPaths = require('../config/paths') * Used to cache slow operations * * See `config/jest/globals.mjs` + * + * @type {{ + * directories?: Map, + * componentsData?: ComponentData[] + * }} */ const cache = global.cache || {} @@ -27,7 +32,6 @@ const cache = global.cache || {} */ const getListing = async (directoryPath, pattern = '**/*', options = {}) => { const listing = await glob(pattern, { - allowEmpty: true, cwd: directoryPath, matchBase: true, nodir: true, @@ -51,7 +55,7 @@ const getDirectories = (directoryPath) => { // Retrieve from cache if (directories) { - return directories + return Promise.resolve(directories) } // Read from disk @@ -66,12 +70,11 @@ const getDirectories = (directoryPath) => { * @returns {function(string): boolean} Returns true for files matching every pattern */ const filterPath = (patterns) => (entryPath) => { - const isMatch = (pattern) => minimatch(entryPath, pattern, { - matchBase: true - }) - - // Return true for files matching every pattern - return patterns.every(isMatch) + return patterns.every( + (pattern) => minimatch(entryPath, pattern, { + matchBase: true + }) + ) } /** @@ -107,16 +110,12 @@ const getComponentData = async (componentName) => { } // Read from disk - try { - const yamlPath = join(configPaths.components, componentName, `${componentName}.yaml`) - const yamlData = yaml.load(await readFile(yamlPath, 'utf8'), { json: true }) + const yamlPath = join(configPaths.components, componentName, `${componentName}.yaml`) + const yamlData = yaml.load(await readFile(yamlPath, 'utf8'), { json: true }) - return { - name: componentName, - ...yamlData - } - } catch (error) { - throw new Error(error) + return { + name: componentName, + ...yamlData } } @@ -177,7 +176,7 @@ module.exports = { * Directory entry path mapper callback * * @callback mapPathToCallback - * @param {ParsedPath} file - Parsed file + * @param {import('path').ParsedPath} file - Parsed file * @returns {string[]} Returns path (or array of paths) */ @@ -186,12 +185,33 @@ module.exports = { * * @typedef {object} ComponentData * @property {string} name - Component name - * @property {unknown[]} [params] - Nunjucks macro options - * @property {unknown[]} [examples] - Example Nunjucks macro options + * @property {ComponentOption[]} [params] - Nunjucks macro options + * @property {ComponentExample[]} [examples] - Example Nunjucks macro options * @property {string} [previewLayout] - Nunjucks layout for component preview * @property {string} [accessibilityCriteria] - Accessibility criteria */ +/** + * Component option from YAML + * + * @typedef {object} ComponentOption + * @property {string} name - Option name + * @property {string} type - Option type + * @property {boolean} required - Option required + * @property {string} description - Option description + * @property {boolean} [isComponent] - Option is another component + * @property {ComponentOption[]} [params] - Nested Nunjucks macro options + */ + +/** + * Component example from YAML + * + * @typedef {object} ComponentExample + * @property {string} name - Example name + * @property {object} data - Example data + * @property {boolean} [hidden] - Example hidden from review app + */ + /** * Full page example from front matter * diff --git a/lib/helper-functions.js b/lib/helper-functions.js index 77f0f76842..014a0a0731 100644 --- a/lib/helper-functions.js +++ b/lib/helper-functions.js @@ -67,15 +67,11 @@ function componentPathToModuleName (componentPath) { * the project root node_modules * * @param {string} packageName - Installed npm package name - * @param {...string} [paths] - Optional child directory paths + * @param {string} [childPath] - Optional child directory path * @returns {string} Path to installed npm package */ -function packageNameToPath (packageName, ...paths) { - const packagePath = dirname(require.resolve(`${packageName}/package.json`)) - - return paths?.length - ? join(packagePath, ...paths) - : packagePath +function packageNameToPath (packageName, childPath = '') { + return join(dirname(require.resolve(`${packageName}/package.json`)), childPath) } module.exports = { diff --git a/lib/jest-helpers.js b/lib/jest-helpers.js index c1c9f6c0eb..f2129f7536 100644 --- a/lib/jest-helpers.js +++ b/lib/jest-helpers.js @@ -109,11 +109,12 @@ function renderTemplate (context = {}, blocks = {}) { * Get examples from a component's metadata file * * @param {string} componentName - Component name - * @returns {Promise>} returns object that includes all examples at once + * @returns {Promise>} returns object that includes all examples at once */ async function getExamples (componentName) { const componentData = await getComponentData(componentName) + /** @type {Object} */ const examples = {} for (const example of componentData?.examples || []) { @@ -177,3 +178,7 @@ module.exports = { renderSass, renderTemplate } + +/** + * @typedef {import('./file-helper.js').ComponentExample} ComponentExample + */ From 4d6762bee7abbda7e2f9625acf350fdafb5c3151 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 15 Dec 2022 17:19:29 +0000 Subject: [PATCH 11/15] Fix various param/return types in tooling --- config/jest/environment/jsdom.mjs | 2 +- config/jest/environment/puppeteer.mjs | 2 +- lib/file-helper.unit.test.js | 2 +- lib/jest-helpers.js | 12 +++--------- lib/puppeteer-helpers.js | 22 +++++++++++++--------- tasks/screenshot-components.mjs | 3 +-- 6 files changed, 20 insertions(+), 23 deletions(-) diff --git a/config/jest/environment/jsdom.mjs b/config/jest/environment/jsdom.mjs index ebf4048ec1..d6e73e33a7 100644 --- a/config/jest/environment/jsdom.mjs +++ b/config/jest/environment/jsdom.mjs @@ -14,7 +14,7 @@ class BrowserVirtualEnvironment extends TestEnvironment { const { virtualConsole } = this.dom // Ensure test fails for browser exceptions - virtualConsole.on('jsdomError', (error) => process.emit('error', error)) + virtualConsole.on('jsdomError', (error) => process.emit('uncaughtException', error)) // Add shared test globals // componentsData, componentsDirectory, examplesDirectory diff --git a/config/jest/environment/puppeteer.mjs b/config/jest/environment/puppeteer.mjs index 3fc2115c11..694440e456 100644 --- a/config/jest/environment/puppeteer.mjs +++ b/config/jest/environment/puppeteer.mjs @@ -19,7 +19,7 @@ class BrowserAutomationEnvironment extends PuppeteerEnvironment { delete error.stack // Ensure test fails - process.emit('error', error) + process.emit('uncaughtException', error) }) // Add shared test globals diff --git a/lib/file-helper.unit.test.js b/lib/file-helper.unit.test.js index 2c5b547b27..07bfcaf82d 100644 --- a/lib/file-helper.unit.test.js +++ b/lib/file-helper.unit.test.js @@ -4,7 +4,7 @@ describe('getComponentData', () => { it('rejects if unable to load component data', async () => { await expect(fileHelper.getComponentData('not-a-real-component')) .rejects - .toThrow('Error: ENOENT: no such file or directory') + .toThrow(/ENOENT: no such file or directory/) }) it('outputs objects with an array of params and examples', async () => { diff --git a/lib/jest-helpers.js b/lib/jest-helpers.js index f2129f7536..5a90286861 100644 --- a/lib/jest-helpers.js +++ b/lib/jest-helpers.js @@ -28,13 +28,7 @@ const nunjucksEnv = nunjucks.configure([join(configPaths.src, 'govuk'), configPa * Nunjucks call tag, with the callBlock passed as the contents of the block * @returns {string} HTML rendered by the macro */ -function renderHtml (componentName, options, callBlock = false) { - if (typeof options === 'undefined') { - throw new Error( - 'Parameters passed to `render` should be an object but are undefined' - ) - } - +function renderHtml (componentName, options, callBlock) { const macroName = componentNameToMacroName(componentName) const macroPath = `${componentName}/macro.njk` @@ -50,7 +44,7 @@ function renderHtml (componentName, options, callBlock = false) { * @param {string} [callBlock] - Content for an optional callBlock, if necessary for the macro to receive one * @returns {string} The result of calling the macro */ -function callMacro (macroName, macroPath, params = [], callBlock = false) { +function callMacro (macroName, macroPath, params = [], callBlock) { const macroParams = params.map(param => JSON.stringify(param, null, 2)).join(',') let macroString = `{%- from "${macroPath}" import ${macroName} -%}` @@ -77,7 +71,7 @@ function callMacro (macroName, macroPath, params = [], callBlock = false) { * Nunjucks call tag, with the callBlock passed as the contents of the block * @returns {import('cheerio').CheerioAPI} a jQuery-like representation of the macro output */ -function render (componentName, options, callBlock = false) { +function render (componentName, options, callBlock) { return cheerio.load( renderHtml(componentName, options, callBlock) ) diff --git a/lib/puppeteer-helpers.js b/lib/puppeteer-helpers.js index cc1fabeedb..078b924aa0 100644 --- a/lib/puppeteer-helpers.js +++ b/lib/puppeteer-helpers.js @@ -23,13 +23,13 @@ const PORT = process.env.PORT || config.ports.test * @param {object} options.nunjucksParams - Params passed to the Nunjucks macro * @param {object} [options.javascriptConfig] - The configuration hash passed * to the component's class for initialisation - * @param {Function} [options.initialiser] - A function that'll run in the + * @param {initialiseCallback} [options.initialiser] - A function that'll run in the * browser to execute arbitrary initialisation. Receives an object with the * passed configuration as `config` and the PascalCased component name as * `componentClassName` * @returns {Promise} Puppeteer page object */ -async function renderAndInitialise (page, componentName, options = {}) { +async function renderAndInitialise (page, componentName, options) { await goTo(page, '/tests/boilerplate') const html = renderHtml(componentName, options.nunjucksParams) @@ -76,11 +76,10 @@ async function goTo (page, path) { * * @param {import('puppeteer').Page} page - Puppeteer page object * @param {string} exampleName - Example name - * @param {import('puppeteer').WaitForOptions} [options] - Navigation options (optional) * @returns {Promise} Puppeteer page object */ -function goToExample (page, exampleName, options) { - return goTo(page, `/examples/${exampleName}`, options) +function goToExample (page, exampleName) { + return goTo(page, `/examples/${exampleName}`) } /** @@ -92,9 +91,9 @@ function goToExample (page, exampleName, options) { * @param {string} options.exampleName - Example name * @returns {Promise} Puppeteer page object */ -function goToComponent (page, componentName, { exampleName } = {}) { - const componentPath = exampleName - ? `/components/${componentName}/${exampleName}/preview` +function goToComponent (page, componentName, options) { + const componentPath = options?.exampleName + ? `/components/${componentName}/${options.exampleName}/preview` : `/components/${componentName}/preview` return goTo(page, componentPath) @@ -128,7 +127,7 @@ function getAttribute ($element, attributeName) { * * @param {import('puppeteer').Page} page - Puppeteer page object * @param {import('puppeteer').ElementHandle} $element - Puppeteer element handle - * @returns {string} The element's accessible name + * @returns {Promise} The element's accessible name * @throws {TypeError} If the element has no corresponding node in the accessibility tree */ async function getAccessibleName (page, $element) { @@ -161,3 +160,8 @@ module.exports = { getAccessibleName, isVisible } + +/** + * @callback initialiseCallback + * @param {{ config: object, componentClassName: string }} context - Initialiser context + */ diff --git a/tasks/screenshot-components.mjs b/tasks/screenshot-components.mjs index 6f28bef4f8..0bcdc59193 100644 --- a/tasks/screenshot-components.mjs +++ b/tasks/screenshot-components.mjs @@ -8,7 +8,6 @@ import { downloadBrowser } from 'puppeteer/lib/esm/puppeteer/node/install.js' import configPaths from '../config/paths.js' import { getDirectories, getListing } from '../lib/file-helper.js' import { goToComponent } from '../lib/puppeteer-helpers.js' -import configPuppeteer from '../puppeteer.config.js' /** * Send all component screenshots to Percy @@ -17,7 +16,7 @@ import configPuppeteer from '../puppeteer.config.js' * @returns {Promise} */ export async function screenshotComponents () { - const browser = await launch(configPuppeteer.launch) + const browser = await launch() const componentNames = await getDirectories(configPaths.components) // Screenshot each component From c418bbea308b94454e8f10057445c9dd4377b8e4 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 19 Jan 2023 18:19:20 +0000 Subject: [PATCH 12/15] Fix various param/return types in source Regarding fallback value types * Preferring `null` for DOM method fallbacks * Preferring `undefined` for string fallbacks --- src/govuk/common/closest-attribute-value.mjs | 10 ++-- .../closest-attribute-value.unit.test.mjs | 4 +- .../error-summary/error-summary.mjs | 10 ++-- src/govuk/components/skip-link/skip-link.mjs | 9 ++-- src/govuk/components/tabs/tabs.mjs | 10 ++-- src/govuk/i18n.mjs | 50 +++++++++++-------- 6 files changed, 52 insertions(+), 41 deletions(-) diff --git a/src/govuk/common/closest-attribute-value.mjs b/src/govuk/common/closest-attribute-value.mjs index 94fd32cab1..df05cb728d 100644 --- a/src/govuk/common/closest-attribute-value.mjs +++ b/src/govuk/common/closest-attribute-value.mjs @@ -5,11 +5,11 @@ import '../vendor/polyfills/Element/prototype/closest.mjs' * * @param {HTMLElement} $element - The element to start walking the DOM tree up * @param {string} attributeName - The name of the attribute - * @returns {string | undefined} Attribute value + * @returns {string | null} Attribute value */ export function closestAttributeValue ($element, attributeName) { - var closestElementWithAttribute = $element.closest('[' + attributeName + ']') - if (closestElementWithAttribute) { - return closestElementWithAttribute.getAttribute(attributeName) - } + var $closestElementWithAttribute = $element.closest('[' + attributeName + ']') + return $closestElementWithAttribute + ? $closestElementWithAttribute.getAttribute(attributeName) + : null } diff --git a/src/govuk/common/closest-attribute-value.unit.test.mjs b/src/govuk/common/closest-attribute-value.unit.test.mjs index d9b7f8a7ab..c2b3851ec5 100644 --- a/src/govuk/common/closest-attribute-value.unit.test.mjs +++ b/src/govuk/common/closest-attribute-value.unit.test.mjs @@ -25,11 +25,11 @@ describe('closestAttributeValue', () => { expect(closestAttributeValue($element, 'lang')).toEqual('en-GB') }) - it('returns undefined if neither the element or a parent have the attribute', () => { + it('returns null if neither the element or a parent have the attribute', () => { const $parent = document.createElement('div') const $element = document.createElement('div') $parent.appendChild($element) - expect(closestAttributeValue($element, 'lang')).toBeUndefined() + expect(closestAttributeValue($element, 'lang')).toBeNull() }) }) diff --git a/src/govuk/components/error-summary/error-summary.mjs b/src/govuk/components/error-summary/error-summary.mjs index 7cbb785fb1..e051383b77 100644 --- a/src/govuk/components/error-summary/error-summary.mjs +++ b/src/govuk/components/error-summary/error-summary.mjs @@ -112,6 +112,10 @@ ErrorSummary.prototype.focusTarget = function ($target) { } var inputId = this.getFragmentFromUrl($target.href) + if (!inputId) { + return false + } + var $input = document.getElementById(inputId) if (!$input) { return false @@ -138,11 +142,11 @@ ErrorSummary.prototype.focusTarget = function ($target) { * the hash. * * @param {string} url - URL - * @returns {string} Fragment from URL, without the hash + * @returns {string | undefined} Fragment from URL, without the hash */ ErrorSummary.prototype.getFragmentFromUrl = function (url) { if (url.indexOf('#') === -1) { - return false + return undefined } return url.split('#').pop() @@ -160,7 +164,7 @@ ErrorSummary.prototype.getFragmentFromUrl = function (url) { * - The closest parent `