Skip to content

Conversation

@malcolmbaig
Copy link
Contributor

@malcolmbaig malcolmbaig commented Dec 2, 2025

Description

Add note creation UI to the Note tab on the appointment page.

Screenshot 2025-12-02 at 14 26 33 Screenshot 2025-12-03 at 12 53 01

Jira link

https://nhsd-jira.digital.nhs.uk/browse/DTOSS-11596

Review notes

Individual commits are worth a read through for the indirectly related changes — the system test reorg and changes to the validation error helper for system tests.

Review checklist

  • Check database queries are correctly scoped to current_provider

@malcolmbaig malcolmbaig requested a review from a team as a code owner December 2, 2025 14:27
@malcolmbaig malcolmbaig force-pushed the 11596-add-appointment-note branch from 6efc53c to 7b64e2a Compare December 2, 2025 15:18
@edwardhorsford
Copy link
Contributor

I think I see two potential issues in these screenshots

The error summary is showing beneath the tabs, but this is unconventional. Usually they'd be at the top of the page, below any back links as the fist item within main. I think we should first think through where the main element should beset be placed on this page and then look at where the error summary goes. cc @colinrotherham

The left red border on the textArea doesn't look right - it's extending too far below the field. The error bar is expected to align with the bottom of the field. This makes me think there may be a bug in the component or the error is being applied to the wrong thing. The spacing between the label / hint / message looks a bit large to me - I wouldn't mind checking the rendered output for this.

@MatMoore
Copy link
Contributor

MatMoore commented Dec 3, 2025

I think the red border extending too far is because this field is now a character-count component, and we are using the design system as it comes rather than tweaking any of the margins.

By default, it seems to have a large bottom margin, I'm not sure why.

@edwardhorsford
Copy link
Contributor

I think the red border extending too far is because this field is now a character-count component, and we are using the design system as it comes rather than tweaking any of the margins.

Ah if it's the character count (which makes sense!), I guess it's leaving space for the message. That might imply the actual bug is that the default message of You have x characters remaining is missing?

@edwardhorsford
Copy link
Contributor

Checked in the prototype, and the extended error line is the default for character count when using threshold. I reckon it looks a bit messy, but it means that's not a bug on our side.

@malcolmbaig
Copy link
Contributor Author

Checked in the prototype, and the extended error line is the default for character count when using threshold. I reckon it looks a bit messy, but it means that's not a bug on our side.

We have threshold set to 25 currently https://github.com/NHSDigital/dtos-manage-breast-screening/blob/11596-add-appointment-note/manage_breast_screening/nhsuk_forms/fields/char_field.py#L21.

Could set this to 0 in just this form to make the 'characters remaining' message always appear? @edwardhorsford

@github-actions
Copy link

github-actions bot commented Dec 3, 2025

The review app at this URL has been deleted:
https://pr-797.manage-breast-screening.non-live.screening.nhs.uk

Each Appointment has one AppointmentNote. Add the model definition,
migration and factory for this new model.
- Simplify some filenames
- Move CIS2-related system tests into general folder
- Move 'appointment cannot go ahead form' into clinical folder
Amend the implementation of this system test helper so that it also
works with fields that aren't wrapped in a fieldset (eg - a simple text
field).
@edwardhorsford
Copy link
Contributor

We have threshold set to 25 currently https://github.com/NHSDigital/dtos-manage-breast-screening/blob/11596-add-appointment-note/manage_breast_screening/nhsuk_forms/fields/char_field.py#L21.

Could set this to 0 in just this form to make the 'characters remaining' message always appear? @edwardhorsford

@rivalee any views? I somewhat reckon we should set the character limit quite high, and the threshold before any warning is shown also quite high - perhaps 75%. The error line extending too far is mildly annoying, but probably something to fix in the source library or else ignore.

@malcolmbaig
Copy link
Contributor Author

Merging for now, can pick up amendments to character fields in a followup PR.

@malcolmbaig malcolmbaig merged commit 8854484 into main Dec 5, 2025
13 checks passed
@malcolmbaig malcolmbaig deleted the 11596-add-appointment-note branch December 5, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants