Stepper component for use on the additional image details form#955
Stepper component for use on the additional image details form#955
Conversation
5109562 to
6f1f116
Compare
6f1f116 to
707124a
Compare
|
The review app at this URL has been deleted: |
707124a to
fd4995b
Compare
fd4995b to
2d0bd7a
Compare
2d0bd7a to
cd6d31c
Compare
|
|
||
| // Polyfill default value on step up | ||
| if (isEmpty && event?.currentTarget === this.$buttonStepUp) { | ||
| $input.valueAsNumber = min === 0 ? 1 : min |
There was a problem hiding this comment.
this is the polyfill I changed. If the input is blank and min is 0, step up sets it to 1, not 0.
|
|
||
| beforeEach(async () => { | ||
| document.body.innerHTML = ` | ||
| <div class="nhsuk-form-group app-stepper-input" data-module="app-stepper-input" data-max="20" data-min="0"> |
There was a problem hiding this comment.
it's awkward having to specify all this HTML here... for the nhsuk-frontend version, we can just render the nunjucks macro. I considered bringing in nunjucks as a dependency and rendering the jinja component as if it was nunjucks, but I thought this would complicate things too much.
There was a problem hiding this comment.
There's a PR open here to make the components.render() JavaScript API we use (internally) on NHS.UK frontend available for use via the npm package:
I've not had another JavaScript developer available to review it
But would be useful here!
| expect(liveRegion.innerText).toBe('2') | ||
| }) | ||
|
|
||
| it('assumes the min value when stepping down if the input is empty', async () => { |
There was a problem hiding this comment.
Note: these two tests are technically testing jsdom, as this behaviour is normally provided by a browser, but I thought it was worth including as it helps to document the intended behaviour.
| .nhsuk-form-group .app-button--link { | ||
| display: block; | ||
| } | ||
|
|
There was a problem hiding this comment.
This scss is all unchanged from the nhsuk-frontend PR, excpept I've renamed the class prefixes to app-
| @@ -0,0 +1,40 @@ | |||
| {# | |||
There was a problem hiding this comment.
This might become obsolete soon if the icons are accepted into the design system, but I copied the macro structure as I thought it would be handy to have. If we need another custom SVG at any point we can just add it here.
bc8c847 to
97dd7a2
Compare
|
|
||
| def test_renders_stepper_input(self, form_class): | ||
| actual = form_class()["stepper_field"].as_field_group() | ||
| expected = render_to_string( |
There was a problem hiding this comment.
I figured out I could render the underlying component like this to simplify the test. This means the test is not sensitive to the implementation of the jinja component, only that we pass through the right params from the field to the component.
We should do this for the other fields too but I didn't want to overload this PR.
manage_breast_screening/nhsuk_forms/jinja2/forms/stepper-input.jinja
Outdated
Show resolved
Hide resolved
| "id": "id_field", | ||
| "name": "field", | ||
| "inputmode": "numeric", | ||
| "type": "number", |
There was a problem hiding this comment.
We wouldn't typically use type="number" except with the stepper input where we've taken steps to control the unexpected mousewheel behaviour (see guidance).
Perhaps this highlights the need for a plain (non-stepper) number input?
Otherwise the default should be type="text" with inputmode="numeric" here
Related guidance from the GOV.UK Design System:
Avoid using inputs with a type of number
Do not use
<input type="number">unless your user research shows that there’s a need for it. With<input type="number">there’s a risk of users accidentally incrementing a number when they’re trying to do something else - for example, scroll up or down the page. And if the user tries to enter something that’s not a number, there’s no explicit feedback about what they’re doing wrong.
There was a problem hiding this comment.
I think the simplest thing then is to change the default case of IntegerField to output a text field. We are using this in a few forms already for entering years.
This behaviour comes from the NumberInput widget which is the django default for IntegerField, but we can override it to use text instead.
manage_breast_screening/nonprod/jinja2/nonprod/components.jinja
Outdated
Show resolved
Hide resolved
manage_breast_screening/nonprod/jinja2/nonprod/components.jinja
Outdated
Show resolved
Hide resolved
manage_breast_screening/nonprod/jinja2/nonprod/components.jinja
Outdated
Show resolved
Hide resolved
40d8882 to
0b977a7
Compare
This repurposes Colin's proposed code for the design system. See nhsuk/nhsuk-frontend#1719 I've stripped out a few options that we don't need right now in order to focus on the use case we have. Should this be accepted into the design system, we'll back out these changes, but the way we call the component will be very similar.
This extends our existing version of IntegerField so that it renders with a stepper when the StepperInput widget is specified.
We should not use input type="number" unless we use the browser's scroll behaviour has been overriden. IntegerField by default should use text with inputmode="numeric". The stepper input enhances this with javascript. https://service-manual.nhs.uk/design-system/components/text-input#asking-for-numbers
0b977a7 to
ff58e03
Compare
|
|
||
| stepUpButton = getByLabelText(document.body, 'Increase').parentElement | ||
| stepDownButton = getByLabelText(document.body, 'Decrease').parentElement | ||
| input = getByLabelText(document.body, 'Number of giraffes') |
There was a problem hiding this comment.
If you can add a <form> element we tend to follow this pattern in the JSDOM tests:
$root = /** @type {HTMLElement} */ (
document.querySelector(`[data-module="${StepperInput.moduleName}"]`)
)Then the $root can be used to locate the $input via accessible name:
| input = getByLabelText(document.body, 'Number of giraffes') | |
| $input = getByRole($root, 'textbox', { | |
| name: 'Number of giraffes' | |
| }) |
It's a holdover from jQuery, but prefer input → $input
There was a problem hiding this comment.
the role was spinbutton rather than textbox but have made the change now
There was a problem hiding this comment.
Hmm that means it was type="number" already
For when we contribute these tests into NHS.UK frontend, we'll want to start with the basic server-side rendered HTML using type="text" and let the component JavaScript do the enhancement for us
| let stepDownButton | ||
| let input | ||
|
|
||
| beforeEach(async () => { |
There was a problem hiding this comment.
Don't think we have any awaits inside
| beforeEach(async () => { | |
| beforeEach(() => { |
| describe('stepper-input', () => { | ||
| const user = userEvent.setup() | ||
| let stepUpButton | ||
| let stepDownButton | ||
| let input |
There was a problem hiding this comment.
Just to align with the JSDOM tests in NHS.UK frontend
- Move
useroutside the describe block - Use the
$prefix for DOM elements - Add type comments
You'll see how to locate $root using moduleName in this comment
| describe('stepper-input', () => { | |
| const user = userEvent.setup() | |
| let stepUpButton | |
| let stepDownButton | |
| let input | |
| const user = userEvent.setup() | |
| describe('Stepper input', () => { | |
| /** @type {HTMLElement} */ | |
| let $root | |
| /** @type {HTMLInputElement} */ | |
| let $input | |
| /** @type {HTMLElement} */ | |
| let $stepUpButton | |
| /** @type {HTMLElement} */ | |
| let $stepDownButton |
405f726 to
5f99f98
Compare
|



Description
This brings in a version of the stepper component as
app-stepper-input, and wires it up to the customisedIntegerFieldin the nhsforms app.Example usage:
At the time of writing, the additional image details form is not merged yet, but I can update that to use this component when it is.
Note: I've added a little debug page at /debug/components in order to preview components like this that are not coming from the design system. This is not enabled in the review environments as I don't intend to be visible except for developers running in DEBUG mode. If you prefer, I can remove this page, but I found useful for manual testing.
Jira link
https://nhsd-jira.digital.nhs.uk/browse/DTOSS-12115
Review notes
Post merge tasks
Review checklist