Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
infrastructure/terraform/modules/backend-api/module_get_template_letter_variants_lambda.tf
Outdated
Show resolved
Hide resolved
…CCM-14190_choose-printing-and-postage
…CCM-14190_choose-printing-and-postage
ClareJonesBJSS
force-pushed
the
feature/CCM-14190_choose-printing-and-postage
branch
from
45c6848 to
b43835b
Compare
infrastructure/terraform/modules/backend-api/dynamodb_table_letter_variants.tf
Show resolved
Hide resolved
infrastructure/terraform/modules/backend-api/module_get_letter_variant_lambda.tf
Outdated
Show resolved
Hide resolved
infrastructure/terraform/modules/backend-api/module_get_template_letter_variants_lambda.tf
Outdated
Show resolved
Hide resolved
harrim91
dismissed stale reviews from alexnuttall and aidenvaines-cgi
via
741c034
| const [state] = useNHSNotifyForm(); | ||
|
|
||
| return ( | ||
| <input |
There was a problem hiding this comment.
is there a 1 sentence explanation for why we cant make our notify form component work with the inputs from the react lib?
There was a problem hiding this comment.
In a sentence. The React components are too opinionated for our designs. Our designs tend to go off-piste a little bit, and this approach gives you smaller components which have the NHS styling, but which are more composable.
It depends on the input type and the context we're using it in. I
n this case, the react lib Radios component always looks something like this.
But in this new page, we needed them to be nested inside a table. This was the simplest way to achieve that.
Our file upload just looks completely different to the NHSUK one (not sure why we didn't go with the standard one)
The library components support props for showing errors, hints etc. But we have instances where it's laid out differently. e.g. in some places we have a <details> section between the hint and the input, which isn't supported by the lib.
| } | ||
|
|
||
| const searchParams = await props.searchParams; | ||
| const lockNumberResult = $LockNumber.safeParse(searchParams?.lockNumber); |
There was a problem hiding this comment.
on the other pages, have you not redirected early on the invalid lock number? pre fetching client?
There was a problem hiding this comment.
maybe the same Q also fcor the template status?
I guess its keeping all those conditions for the redirect together vs redirecting early as soon as we know?
There was a problem hiding this comment.
It's the same as this in some places, and there's an early redirect in other places. I think in this case it was to group all of the conditions for the redirect to the preview page together, and because there are two possible preview pages based on the template status.
I can put the early redirect in if you think it's better. I'd have to get the template status though to know which preview url to redirect to. Unless the preview -> preview-submitted status check / redirect goes into the preview page?
| value={variant.id} | ||
| aria-labelledby={`label-${variant.id}`} | ||
| /> | ||
| <span className='nhsuk-radios__label'> </span> |
There was a problem hiding this comment.
is this just to make the nhs frontend styling work?
There was a problem hiding this comment.
Yeah I think the radio button doesn't show without this, I took this from somewhere else I think.
Description
The other big change is replacing
createAuthHelperin the playwright tests with a broadergetTestContextfunction. Previously the client config set up was nested into the AuthHelper. Instead of also nesting the new letter variants helper (which also needs to be globally available state, linked to clients), I've un-nested things, and added theAuthContextclass which is like a facade, responsible for exposing the three things, and orchestrating the setup / teardown. This change is where the bulk of the changed files comes from.@ClareJonesBJSS edit - also tidied up the barrels in nhs-notify-backend-client which has ballooned the files touched again, but just with import updates
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.