Skip to content

Commit

Permalink
Add $module + element instanceof checks
Browse files Browse the repository at this point in the history
Review component JSDoc types against module and element checks

We now prefer `{Element}` type to `{HTMLElement}` to maintain compatibility with `querySelector` but we continue to use `instanceof` to guard types

Co-Authored-By: Romaric <romaric.pascal@digital.cabinet-office.gov.uk>
  • Loading branch information
colinrotherham and romaricpascal committed Jan 16, 2023
1 parent 11224b3 commit aa023c4
Show file tree
Hide file tree
Showing 12 changed files with 33 additions and 30 deletions.
2 changes: 1 addition & 1 deletion docs/contributing/coding-standards/js.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import '../../vendor/polyfills/Element.mjs'
* @param {Element} $module - HTML element to use for component
*/
function Example ($module) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Example($module).init()`
return this
Expand Down
12 changes: 7 additions & 5 deletions src/govuk/components/accordion/accordion.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ var ACCORDION_TRANSLATIONS = {
* @param {AccordionConfig} [config] - Accordion config
*/
function Accordion ($module, config) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Accordion($module).init()`
return this
Expand Down Expand Up @@ -299,7 +299,9 @@ Accordion.prototype.constructHeaderMarkup = function ($header, index) {
*/
Accordion.prototype.onBeforeMatch = function (event) {
var $fragment = event.target
if (!$fragment) {

// Handle elements with `.closest()` support only
if (!($fragment instanceof Element)) {
return
}

Expand Down Expand Up @@ -362,7 +364,7 @@ Accordion.prototype.setExpanded = function (expanded, $section) {
var $content = $section.querySelector('.' + this.sectionContentClass)

if (!$showHideIcon ||
!$showHideText ||
!($showHideText instanceof HTMLElement) ||
!$button ||
!$content) {
return
Expand All @@ -379,12 +381,12 @@ Accordion.prototype.setExpanded = function (expanded, $section) {
var ariaLabelParts = []

var $headingText = $section.querySelector('.' + this.sectionHeadingTextClass)
if ($headingText) {
if ($headingText instanceof HTMLElement) {
ariaLabelParts.push($headingText.innerText.trim())
}

var $summary = $section.querySelector('.' + this.sectionSummaryClass)
if ($summary) {
if ($summary instanceof HTMLElement) {
ariaLabelParts.push($summary.innerText.trim())
}

Expand Down
4 changes: 2 additions & 2 deletions src/govuk/components/button/button.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ var DEBOUNCE_TIMEOUT_IN_SECONDS = 1
* @param {ButtonConfig} [config] - Button config
*/
function Button ($module, config) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Button($module).init()`
return this
Expand Down Expand Up @@ -74,7 +74,7 @@ Button.prototype.handleKeyDown = function (event) {
}

// Handle elements with [role="button"] only
if ($target.getAttribute('role') === 'button') {
if ($target instanceof HTMLElement && $target.getAttribute('role') === 'button') {
event.preventDefault() // prevent the page from scrolling
$target.click()
}
Expand Down
4 changes: 2 additions & 2 deletions src/govuk/components/character-count/character-count.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ var CHARACTER_COUNT_TRANSLATIONS = {
* @param {CharacterCountConfig} [config] - Character count config
*/
function CharacterCount ($module, config) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new CharacterCount($module).init()`
return this
}

var $textarea = $module.querySelector('.govuk-js-character-count')
if (!$textarea) {
if (!($textarea instanceof HTMLTextAreaElement)) {
return this
}

Expand Down
2 changes: 1 addition & 1 deletion src/govuk/components/checkboxes/checkboxes.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import '../../vendor/polyfills/Function/prototype/bind.mjs'
* @param {Element} $module - HTML element to use for checkboxes
*/
function Checkboxes ($module) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Checkboxes($module).init()`
return this
Expand Down
9 changes: 5 additions & 4 deletions src/govuk/components/details/details.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ var KEY_SPACE = 32
* @param {Element} $module - HTML element to use for details
*/
function Details ($module) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Details($module).init()`
return this
Expand All @@ -39,7 +39,8 @@ Details.prototype.init = function () {
}

// If there is native details support, we want to avoid running code to polyfill native behaviour.
var hasNativeDetails = typeof this.$module.open === 'boolean'
var hasNativeDetails = 'HTMLDetailsElement' in window &&
this.$module instanceof HTMLDetailsElement

if (!hasNativeDetails) {
this.polyfillDetails()
Expand Down Expand Up @@ -128,7 +129,7 @@ Details.prototype.polyfillHandleInputs = function (callback) {
var $target = event.target
// When the key gets pressed - check if it is enter or space
if (event.keyCode === KEY_ENTER || event.keyCode === KEY_SPACE) {
if ($target.nodeName.toLowerCase() === 'summary') {
if ($target instanceof HTMLElement && $target.nodeName.toLowerCase() === 'summary') {
// Prevent space from scrolling the page
// and enter from submitting a form
event.preventDefault()
Expand All @@ -147,7 +148,7 @@ Details.prototype.polyfillHandleInputs = function (callback) {
this.$summary.addEventListener('keyup', function (event) {
var $target = event.target
if (event.keyCode === KEY_SPACE) {
if ($target.nodeName.toLowerCase() === 'summary') {
if ($target instanceof HTMLElement && $target.nodeName.toLowerCase() === 'summary') {
event.preventDefault()
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/govuk/components/error-summary/error-summary.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import '../../vendor/polyfills/Function/prototype/bind.mjs'
* @param {ErrorSummaryConfig} [config] - Error summary config
*/
function ErrorSummary ($module, config) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new ErrorSummary($module).init()`
return this
Expand Down Expand Up @@ -184,7 +184,7 @@ ErrorSummary.prototype.getAssociatedLegendOrLabel = function ($input) {

// If the input type is radio or checkbox, always use the legend if there
// is one.
if ($input.type === 'checkbox' || $input.type === 'radio') {
if ($input instanceof HTMLInputElement && ($input.type === 'checkbox' || $input.type === 'radio')) {
return $candidateLegend
}

Expand All @@ -211,7 +211,7 @@ ErrorSummary.prototype.getAssociatedLegendOrLabel = function ($input) {

var inputId = $input.getAttribute('id')
var $label = document.querySelector("label[for='" + inputId + "']") || $input.closest('label')
if (!$label) {
if (!($label instanceof HTMLLabelElement)) {
return
}

Expand Down
2 changes: 1 addition & 1 deletion src/govuk/components/header/header.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import '../../vendor/polyfills/Function/prototype/bind.mjs'
* @param {Element} $module - HTML element to use for header
*/
function Header ($module) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Header($module).init()`
return this
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import '../../vendor/polyfills/Event.mjs' // addEventListener, event.target norm
* @param {NotificationBannerConfig} [config] - Notification banner config
*/
function NotificationBanner ($module, config) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new NotificationBanner($module).init()`
return this
Expand Down
2 changes: 1 addition & 1 deletion src/govuk/components/radios/radios.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import '../../vendor/polyfills/Function/prototype/bind.mjs'
* @param {Element} $module - HTML element to use for radios
*/
function Radios ($module) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Radios($module).init()`
return this
Expand Down
2 changes: 1 addition & 1 deletion src/govuk/components/skip-link/skip-link.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import '../../vendor/polyfills/Function/prototype/bind.mjs'
* @param {Element} $module - HTML element to use for skip link
*/
function SkipLink ($module) {
if (!$module) {
if (!($module instanceof HTMLAnchorElement)) {
// Return instance for method chaining
// using `new SkipLink($module).init()`
return this
Expand Down
16 changes: 8 additions & 8 deletions src/govuk/components/tabs/tabs.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ import '../../vendor/polyfills/Function/prototype/bind.mjs'
* @param {Element} $module - HTML element to use for tabs
*/
function Tabs ($module) {
if (!$module) {
if (!($module instanceof HTMLElement)) {
// Return instance for method chaining
// using `new Tabs($module).init()`
return this
}

var $tabs = $module.querySelectorAll('.govuk-tabs__tab')
var $tabs = $module.querySelectorAll('a.govuk-tabs__tab')
if (!$tabs.length) {
return this
}
Expand Down Expand Up @@ -139,7 +139,7 @@ Tabs.prototype.teardown = function () {
var $module = this.$module
var $tabs = this.$tabs
var $tabList = $module.querySelector('.govuk-tabs__list')
var $tabListItems = $module.querySelectorAll('.govuk-tabs__list-item')
var $tabListItems = $module.querySelectorAll('a.govuk-tabs__list-item')

if (!$tabs || !$tabList || !$tabListItems) {
return
Expand Down Expand Up @@ -230,7 +230,7 @@ Tabs.prototype.showTab = function ($tab) {
* @returns {HTMLAnchorElement | undefined} Tab link
*/
Tabs.prototype.getTab = function (hash) {
var $tab = this.$module.querySelector('.govuk-tabs__tab[href="' + hash + '"]')
var $tab = this.$module.querySelector('a.govuk-tabs__tab[href="' + hash + '"]')
if (!$tab) {
return
}
Expand Down Expand Up @@ -297,7 +297,7 @@ Tabs.prototype.onTabClick = function (event) {
var $currentTab = this.getCurrentTab()
var $nextTab = event.target

if (!$currentTab || !$nextTab) {
if (!$currentTab || !($nextTab instanceof HTMLAnchorElement)) {
return
}

Expand Down Expand Up @@ -373,7 +373,7 @@ Tabs.prototype.activateNextTab = function () {
return
}

var $nextTab = $nextTabListItem.querySelector('.govuk-tabs__tab')
var $nextTab = $nextTabListItem.querySelector('a.govuk-tabs__tab')
if (!$nextTab) {
return
}
Expand All @@ -398,7 +398,7 @@ Tabs.prototype.activatePreviousTab = function () {
return
}

var $previousTab = $previousTabListItem.querySelector('.govuk-tabs__tab')
var $previousTab = $previousTabListItem.querySelector('a.govuk-tabs__tab')
if (!$previousTab) {
return
}
Expand Down Expand Up @@ -488,7 +488,7 @@ Tabs.prototype.highlightTab = function ($tab) {
* @returns {HTMLAnchorElement | undefined} Tab link
*/
Tabs.prototype.getCurrentTab = function () {
var $tab = this.$module.querySelector('.govuk-tabs__list-item--selected .govuk-tabs__tab')
var $tab = this.$module.querySelector('.govuk-tabs__list-item--selected a.govuk-tabs__tab')
if (!$tab) {
return
}
Expand Down

0 comments on commit aa023c4

Please sign in to comment.