Skip to content

Fix DF 529: location cosmetics #260

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.

Sign up for GitHub

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

Merged
mokhld merged 5 commits into from
Nov 20, 2025
Merged

Fix DF 529: location cosmetics #260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
92be527
fix: location field accordion spacing and markdown rendering
mokhld Nov 20, 2025
580623e
fix: preserve acronyms in location field error messages
mokhld Nov 20, 2025
18e320c
test: use realistic titles in location field tests
mokhld Nov 20, 2025
2cdd58d
test: add v2 location output formatter tests
mokhld Nov 20, 2025
9d2cf89
test: fixed failing test
mokhld Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/client/stylesheets/shared.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,7 @@
content: none;
}
}

Copy link
Copy Markdown
Contributor

@jbarnsley10 jbarnsley10 Nov 20, 2025

Choose a reason for hiding this comment

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

I assume this doesn't have an impact on other components?

Copy link
Copy Markdown
Contributor Author

@mokhld mokhld Nov 20, 2025

Choose a reason for hiding this comment

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

Not that I can find. This only targets nested form groups because regular form fields don't use nested form groups. This is specifically needed for the location field components because we're wrapping the text field inside another form group for the proper error styling. It's a strange one.

.govuk-form-group .govuk-form-group {
margin-bottom: 0;
}
24 changes: 18 additions & 6 deletions src/server/plugins/engine/components/LocationFieldBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
isFormValue
} from '~/src/server/plugins/engine/components/FormComponent.js'
import { addClassOptionIfNone } from '~/src/server/plugins/engine/components/helpers/index.js'
import { markdown } from '~/src/server/plugins/engine/components/markdownParser.js'
import { messageTemplate } from '~/src/server/plugins/engine/pageControllers/validationOptions.js'
import {
type ErrorMessageTemplateList,
Expand All @@ -28,6 +27,7 @@ interface LocationFieldOptions {
interface ValidationConfig {
pattern: RegExp
patternErrorMessage: string
requiredMessage?: string
}

/**
Expand Down Expand Up @@ -58,16 +58,22 @@ export abstract class LocationFieldBase extends FormComponent {
addClassOptionIfNone(locationOptions, 'govuk-input--width-10')

const config = this.getValidationConfig()
const requiredMessage =
config.requiredMessage ?? (messageTemplate.required as string)

const messages: LanguageMessages = {
'any.required': requiredMessage,
'string.empty': requiredMessage,
'string.pattern.base': config.patternErrorMessage
}

let formSchema = joi
.string()
.trim()
.label(this.label)
.required()
.pattern(config.pattern)
.messages({
'string.pattern.base': config.patternErrorMessage
})
.messages(messages)

if (locationOptions.required === false) {
formSchema = formSchema.allow('')
Expand Down Expand Up @@ -115,17 +121,23 @@ export abstract class LocationFieldBase extends FormComponent {
if (this.instructionText) {
return {
...viewModel,
instructionText: markdown.parse(this.instructionText, { async: false })
instructionText: this.instructionText
}
}

return viewModel
}

getAllPossibleErrors(): ErrorMessageTemplateList {
const config = this.getValidationConfig()

return {
baseErrors: [
{ type: 'required', template: messageTemplate.required },
{
type: 'required',
template:
config.requiredMessage ?? (messageTemplate.required as string)
},
...this.getErrorTemplates()
],
advancedSettingsErrors: []
Expand Down
5 changes: 1 addition & 4 deletions src/server/plugins/engine/components/LocationFieldHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { type Context, type CustomValidator } from 'joi'
import { type EastingNorthingField } from '~/src/server/plugins/engine/components/EastingNorthingField.js'
import { isFormValue } from '~/src/server/plugins/engine/components/FormComponent.js'
import { type LatLongField } from '~/src/server/plugins/engine/components/LatLongField.js'
import { markdown } from '~/src/server/plugins/engine/components/markdownParser.js'
import {
type DateInputItem,
type Label,
Expand Down Expand Up @@ -174,9 +173,7 @@ export function getLocationFieldViewModel(
if (component.options.instructionText) {
return {
...result,
instructionText: markdown.parse(component.options.instructionText, {
async: false
})
instructionText: component.options.instructionText
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/server/plugins/engine/components/NationalGridFieldNumberField.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ describe('NationalGridFieldNumberField', () => {

expect(result.errors).toEqual([
expect.objectContaining({
text: 'Enter example National Grid field number'
text: 'Enter Example National Grid field number'
})
])
})
Expand Down Expand Up @@ -293,7 +293,7 @@ describe('NationalGridFieldNumberField', () => {
value: getFormData('NG1234567'),
errors: expect.arrayContaining([
expect.objectContaining({
text: 'Enter a valid National Grid field number for grid field like NG 1234 5678'
text: 'Enter a valid National Grid field number for Grid field like NG 1234 5678'
})
])
}
Expand All @@ -304,7 +304,7 @@ describe('NationalGridFieldNumberField', () => {
value: getFormData('N123456789'),
errors: expect.arrayContaining([
expect.objectContaining({
text: 'Enter a valid National Grid field number for grid field like NG 1234 5678'
text: 'Enter a valid National Grid field number for Grid field like NG 1234 5678'
})
])
}
Expand All @@ -315,7 +315,7 @@ describe('NationalGridFieldNumberField', () => {
value: getFormData('NGABCDEFGH'),
errors: expect.arrayContaining([
expect.objectContaining({
text: 'Enter a valid National Grid field number for grid field like NG 1234 5678'
text: 'Enter a valid National Grid field number for Grid field like NG 1234 5678'
})
])
}
Expand Down
4 changes: 2 additions & 2 deletions src/server/plugins/engine/components/NationalGridFieldNumberField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { type NationalGridFieldNumberFieldComponent } from '@defra/forms-model'
import lowerFirst from 'lodash/lowerFirst.js'

import { LocationFieldBase } from '~/src/server/plugins/engine/components/LocationFieldBase.js'

Expand All @@ -15,7 +14,8 @@ export class NationalGridFieldNumberField extends LocationFieldBase {

return {
pattern,
patternErrorMessage: `Enter a valid National Grid field number for ${lowerFirst(this.label)} like NG 1234 5678`
patternErrorMessage: `Enter a valid National Grid field number for {{#title}} like NG 1234 5678`,
Copy link
Copy Markdown
Contributor

@jbarnsley10 jbarnsley10 Nov 20, 2025

Choose a reason for hiding this comment

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

Are we stripping spaces or insisting on spaces? Or allowing with/without spaces?

Copy link
Copy Markdown
Contributor Author

@mokhld mokhld Nov 20, 2025

Choose a reason for hiding this comment

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

We haven't changed the spaces at all. It still supports both formats. It only changes the error message template syntax for consistency.

requiredMessage: 'Enter {{#title}}'
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/server/plugins/engine/components/OsGridRefField.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ describe('OsGridRefField', () => {

expect(result.errors).toEqual([
expect.objectContaining({
text: 'Enter example OS grid reference'
text: 'Enter Example OS grid reference'
})
])
})
Expand Down Expand Up @@ -308,7 +308,7 @@ describe('OsGridRefField', () => {
value: getFormData('TQ12345'),
errors: expect.arrayContaining([
expect.objectContaining({
text: 'Enter a valid OS grid reference for grid reference like TQ123456'
text: 'Enter a valid OS grid reference for Grid reference like TQ123456'
})
])
}
Expand All @@ -319,7 +319,7 @@ describe('OsGridRefField', () => {
value: getFormData('AA1234567'),
errors: expect.arrayContaining([
expect.objectContaining({
text: 'Enter a valid OS grid reference for grid reference like TQ123456'
text: 'Enter a valid OS grid reference for Grid reference like TQ123456'
})
])
}
Expand All @@ -330,7 +330,7 @@ describe('OsGridRefField', () => {
value: getFormData('TQABCDEF'),
errors: expect.arrayContaining([
expect.objectContaining({
text: 'Enter a valid OS grid reference for grid reference like TQ123456'
text: 'Enter a valid OS grid reference for Grid reference like TQ123456'
})
])
}
Expand Down
4 changes: 2 additions & 2 deletions src/server/plugins/engine/components/OsGridRefField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { type OsGridRefFieldComponent } from '@defra/forms-model'
import lowerFirst from 'lodash/lowerFirst.js'

import { LocationFieldBase } from '~/src/server/plugins/engine/components/LocationFieldBase.js'

Expand All @@ -18,7 +17,8 @@ export class OsGridRefField extends LocationFieldBase {

return {
pattern,
patternErrorMessage: `Enter a valid OS grid reference for ${lowerFirst(this.label)} like TQ123456`
patternErrorMessage: `Enter a valid OS grid reference for {{#title}} like TQ123456`,
requiredMessage: 'Enter {{#title}}'
}
}

Expand Down
Loading
Loading