Skip to content

Commit

Permalink
Update coding standards with JSDoc changes etc
Browse files Browse the repository at this point in the history
Also includes:

1. Checks for `$module` on instantiation
2. Checks for selectors on init
3. Return `this` on init

We prefer `{Element}` type to `{HTMLElement}` etc to maintain compatibility with `querySelector`
  • Loading branch information
colinrotherham committed Jan 18, 2023
1 parent 70c61a5 commit 4bd2dbb
Show file tree
Hide file tree
Showing 16 changed files with 423 additions and 134 deletions.
104 changes: 67 additions & 37 deletions docs/contributing/coding-standards/js.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,53 @@
JavaScript files have the same name as the component's folder name. Test files have a `.test` suffix placed before the file extension.

```
checkboxes
├── checkboxes.mjs
└── checkboxes.test.js
component
├── component.mjs
└── component.test.js
```

## Skeleton

```js
import { nodeListForEach } from '../vendor/common.mjs'
import '../../vendor/polyfills/Element.mjs'

function Checkboxes ($module) {
// code goes here
/**
* Component name
*
* @class
* @param {Element} $module - HTML element to use for component
*/
function Example ($module) {
if (!$module) {
// Return instance for method chaining
// using `new Example($module).init()`
return this
}

this.$module = $module

// Code goes here
}

Checkboxes.prototype.init = function () {
// code goes here
/**
* Initialise component
*
* @returns {Example} Example component
*/
Example.prototype.init = function () {
// Check that required elements are present
if (!this.$module) {
return this
}

// Code goes here

// Return instance for assignment
// `var myExample = new Example($module).init()`
return this
}

export default Checkboxes
export default Example
```

## Use data attributes to initialise component JavaScript
Expand All @@ -48,15 +76,15 @@ Use `/** ... */` for multi-line comments. Include a description, and specify typ

```js
/**
* Get the nearest ancestor element of a node that matches a given tag name
* @param {object} node element
* @param {string} match tag name (e.g. div)
* @return {object} ancestor element
*/

function (node, match) {
// code goes here
return ancestor
* Get the first descendent (child) of an HTML element that matches a given tag name
*
* @param {Element} $element - HTML element
* @param {string} tagName - Tag name (for example 'div')
* @returns {Element} Ancestor element
*/
function ($element, tagName) {
// Code goes here
return $element.querySelector(tagName)
}
```

Expand All @@ -73,52 +101,54 @@ Use the prototype design pattern to structure your code.
Create a constructor and define any variables that the object needs.

```js
function Checkboxes ($module) {
// code goes here
function Example ($module) {
// Code goes here
}
```

Assign methods to the prototype object. Do not overwrite the prototype with a new object as this makes inheritance impossible.

```js
// bad
Checkboxes.prototype = {
// Bad
Example.prototype = {
init: function () {
// code goes here
// Code goes here
}
}

// good
Checkboxes.prototype.init = function () {
// code goes here
// Good
Example.prototype.init = function () {
// Code goes here
}
```

When initialising an object, use the `new` keyword.

```js
// bad
var myCheckbox = Checkbox().init()
// Bad
var myExample = Example().init()

// good
var myCheckbox = new Checkbox().init()
// Good
var myExample = new Example().init()
```

## Modules

Use ES6 modules (`import`/`export`) over a non-standard module system. You can always transpile to your preferred module system.
Use ECMAScript modules (`import`/`export`) over CommonJS and other formats. You can always transpile to your preferred module system.

```js
import { nodeListForEach } from '../vendor/common.mjs'
// code goes here
export default Checkboxes
import { closestAttributeValue } from '../common/index.mjs'

// Code goes here
export function exampleHelper1 () {}
export function exampleHelper2 () {}
```

Avoid using wildcard (`import * as nodeListForEach`) imports.
You must specify the file extension when using the import keyword.

You must specify the file extension for a file when importing it.
Avoid using namespace imports (`import * as namespace`) in code transpiled to CommonJS (or AMD) bundled code as this can prevent "tree shaking" optimisations.

Use default export over named export.
Prefer named exports over default exports to avoid compatibility issues with transpiler "synthetic default" as discussed in: https://github.com/alphagov/govuk-frontend/issues/2829

## Polyfilling

Expand Down
2 changes: 1 addition & 1 deletion src/govuk/all.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export {
* Config for all components
*
* @typedef {object} Config
* @property {HTMLElement} [scope=document] - Scope to query for components
* @property {Element} [scope=document] - Scope to query for components
* @property {import('./components/accordion/accordion.mjs').AccordionConfig} [accordion] - Accordion config
* @property {import('./components/button/button.mjs').ButtonConfig} [button] - Button config
* @property {import('./components/character-count/character-count.mjs').CharacterCountConfig} [characterCount] - Character Count config
Expand Down
2 changes: 1 addition & 1 deletion src/govuk/common/closest-attribute-value.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import '../vendor/polyfills/Element/prototype/closest.mjs'
/**
* Returns the value of the given attribute closest to the given element (including itself)
*
* @param {HTMLElement} $element - The element to start walking the DOM tree up
* @param {Element} $element - The element to start walking the DOM tree up
* @param {string} attributeName - The name of the attribute
* @returns {string | undefined} Attribute value
*/
Expand Down
3 changes: 3 additions & 0 deletions src/govuk/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,13 @@ export function extractConfigByNamespace (configObject, namespace) {
if (!configObject || typeof configObject !== 'object') {
throw new Error('Provide a `configObject` of type "object".')
}

if (!namespace || typeof namespace !== 'string') {
throw new Error('Provide a `namespace` of type "string" to filter the `configObject` by.')
}

var newObject = {}

for (var key in configObject) {
// Split the key into parts, using . as our namespace separator
var keyParts = key.split('.')
Expand Down
72 changes: 55 additions & 17 deletions src/govuk/components/accordion/accordion.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,16 @@ var ACCORDION_TRANSLATIONS = {
* attribute, which also provides accessibility.
*
* @class
* @param {HTMLElement} $module - HTML element to use for accordion
* @param {Element} $module - HTML element to use for accordion
* @param {AccordionConfig} [config] - Accordion config
*/
function Accordion ($module, config) {
if (!$module) {
// Return instance for method chaining
// using `new Accordion($module).init()`
return this
}

this.$module = $module

var defaultConfig = {
Expand Down Expand Up @@ -79,17 +85,24 @@ function Accordion ($module, config) {
this.sectionSummaryFocusClass = 'govuk-accordion__section-summary-focus'
this.sectionContentClass = 'govuk-accordion__section-content'

this.$sections = this.$module.querySelectorAll('.' + this.sectionClass)
var $sections = this.$module.querySelectorAll('.' + this.sectionClass)
if (!$sections.length) {
return this
}

this.$sections = $sections
this.browserSupportsSessionStorage = helper.checkForSessionStorage()
}

/**
* Initialise component
*
* @returns {Accordion} Accordion component
*/
Accordion.prototype.init = function () {
// Check for module
if (!this.$module) {
return
// Check that required elements are present
if (!this.$module || !this.$sections) {
return this
}

this.initControls()
Expand All @@ -98,6 +111,10 @@ Accordion.prototype.init = function () {
// See if "Show all sections" button text should be updated
var areAllSectionsOpen = this.checkIfAllSectionsOpen()
this.updateShowAllButton(areAllSectionsOpen)

// Return instance for assignment
// `var myAccordion = new Accordion($module).init()`
return this
}

/**
Expand Down Expand Up @@ -145,13 +162,17 @@ Accordion.prototype.initSectionHeaders = function () {
/**
* Loop through section headers
*
* @param {HTMLElement} $section - Section element
* @param {Element} $section - Section element
* @param {number} index - Section index
* @this {Accordion}
*/
function ($section, index) {
// Set header attributes
var $header = $section.querySelector('.' + this.sectionHeaderClass)
if (!$header) {
return
}

// Set header attributes
this.constructHeaderMarkup($header, index)
this.setExpanded(this.isExpanded($section), $section)

Expand All @@ -168,14 +189,18 @@ Accordion.prototype.initSectionHeaders = function () {
/**
* Construct section header
*
* @param {HTMLElement} $header - Section header
* @param {Element} $header - Section header
* @param {number} index - Section index
*/
Accordion.prototype.constructHeaderMarkup = function ($header, index) {
var $span = $header.querySelector('.' + this.sectionButtonClass)
var $heading = $header.querySelector('.' + this.sectionHeadingClass)
var $summary = $header.querySelector('.' + this.sectionSummaryClass)

if (!$span || !$heading) {
return
}

// Create a button element that will replace the '.govuk-accordion__section-button' span
var $button = document.createElement('button')
$button.setAttribute('type', 'button')
Expand Down Expand Up @@ -233,7 +258,7 @@ Accordion.prototype.constructHeaderMarkup = function ($header, index) {
$button.appendChild(this.getButtonPunctuationEl())

// If summary content exists add to DOM in correct order
if (typeof ($summary) !== 'undefined' && $summary !== null) {
if ($summary) {
// Create a new `span` element and copy the summary line content from the original `div` to the
// new `span`
// This is because the summary line text is now inside a button element, which can only contain
Expand Down Expand Up @@ -273,7 +298,13 @@ Accordion.prototype.constructHeaderMarkup = function ($header, index) {
* @param {Event} event - Generic event
*/
Accordion.prototype.onBeforeMatch = function (event) {
var $section = event.target.closest('.' + this.sectionClass)
var $fragment = event.target
if (!$fragment) {
return
}

// Handle when fragment is inside section
var $section = $fragment.closest('.' + this.sectionClass)
if ($section) {
this.setExpanded(true, $section)
}
Expand All @@ -282,7 +313,7 @@ Accordion.prototype.onBeforeMatch = function (event) {
/**
* When section toggled, set and store state
*
* @param {HTMLElement} $section - Section element
* @param {Element} $section - Section element
*/
Accordion.prototype.onSectionToggle = function ($section) {
var expanded = this.isExpanded($section)
Expand All @@ -305,7 +336,7 @@ Accordion.prototype.onShowOrHideAllToggle = function () {
/**
* Loop through section headers
*
* @param {HTMLElement} $section - Section element
* @param {Element} $section - Section element
* @this {Accordion}
*/
function ($section) {
Expand All @@ -322,14 +353,21 @@ Accordion.prototype.onShowOrHideAllToggle = function () {
* Set section attributes when opened/closed
*
* @param {boolean} expanded - Section expanded
* @param {HTMLElement} $section - Section element
* @param {Element} $section - Section element
*/
Accordion.prototype.setExpanded = function (expanded, $section) {
var $showHideIcon = $section.querySelector('.' + this.upChevronIconClass)
var $showHideText = $section.querySelector('.' + this.sectionShowHideTextClass)
var $button = $section.querySelector('.' + this.sectionButtonClass)
var $content = $section.querySelector('.' + this.sectionContentClass)

if (!$showHideIcon ||
!$showHideText ||
!$button ||
!$content) {
return
}

var newButtonText = expanded
? this.i18n.t('hideSection')
: this.i18n.t('showSection')
Expand Down Expand Up @@ -381,7 +419,7 @@ Accordion.prototype.setExpanded = function (expanded, $section) {
/**
* Get state of section
*
* @param {HTMLElement} $section - Section element
* @param {Element} $section - Section element
* @returns {boolean} True if expanded
*/
Accordion.prototype.isExpanded = function ($section) {
Expand Down Expand Up @@ -447,7 +485,7 @@ var helper = {
/**
* Set the state of the accordions in sessionStorage
*
* @param {HTMLElement} $section - Section element
* @param {Element} $section - Section element
*/
Accordion.prototype.storeState = function ($section) {
if (this.browserSupportsSessionStorage) {
Expand All @@ -471,7 +509,7 @@ Accordion.prototype.storeState = function ($section) {
/**
* Read the state of the accordions from sessionStorage
*
* @param {HTMLElement} $section - Section element
* @param {Element} $section - Section element
*/
Accordion.prototype.setInitialState = function ($section) {
if (this.browserSupportsSessionStorage) {
Expand All @@ -495,7 +533,7 @@ Accordion.prototype.setInitialState = function ($section) {
* into thematic chunks.
* See https://github.com/alphagov/govuk-frontend/issues/2327#issuecomment-922957442
*
* @returns {HTMLElement} DOM element
* @returns {Element} DOM element
*/
Accordion.prototype.getButtonPunctuationEl = function () {
var $punctuationEl = document.createElement('span')
Expand Down
Loading

0 comments on commit 4bd2dbb

Please sign in to comment.