Conversation
…ocation components
mokhld
force-pushed
the
feat/df-527-location
branch
from
6b33cc0 to
e7a6177
Compare
…improving validation logic
… EastingNorthingField, LatLongField, and OS grid reference validation
|
|
||
| const { name, options, schema } = def | ||
|
|
||
| const isRequired = options.required !== false |
There was a problem hiding this comment.
Sonar doesn't like the negation
There was a problem hiding this comment.
It seemed to not complain about this one.
|
|
||
| protected getValidationConfig() { | ||
| return { | ||
| pattern: /^[A-Z]{2}\d{8}$/i, |
There was a problem hiding this comment.
Is this pattern used anywhere? It may fail if the value hasn't been cleaned (your custom validation cleans before validating)
| def: ComponentDef, | ||
| options: ConstructorParameters<typeof ComponentBase>[1] | ||
| ): Component { | ||
| // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents |
There was a problem hiding this comment.
Is this needed? Was there a type error?
There was a problem hiding this comment.
There was initially, but it's not complaining now so have removed.
| name?: string | ||
| value?: Item['value'] | ||
| classes?: string | ||
| prefix?: ComponentText |
There was a problem hiding this comment.
Why is a prefix/suffix needed on a DateInputItem? Or is this part of your pre/post markdown stuff?
There was a problem hiding this comment.
It's to match to figma designs in that there is a grey degree unit symbol in the input field for lat/ long fields. I'll a comment to make it clearer.
| controller = new PageControllers.SummaryPageController(model, pageDef) | ||
| break | ||
|
|
||
| case ControllerType.SummaryWithConfirmationEmail: |
There was a problem hiding this comment.
I must have missed this when I added the confirmation email. Thanks for fixing
|
|
||
| <div class="govuk-date-input"> | ||
| {% for item in component.model.items %} | ||
| <div class="govuk-date-input__item"> |
There was a problem hiding this comment.
Why a date-input-item?
There was a problem hiding this comment.
Good spot, this was from an earlier draft. It makes sense to split this out into it's own css class and scss sheet.
jbarnsley10
left a comment
jbarnsley10
left a comment
There was a problem hiding this comment.
Really great work. Just a few comments/questions.
|
|
||
| protected getValidationConfig() { | ||
| return { | ||
| pattern: /^[A-Z]{2}\d{6,10}$/i, |
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks @davidjamesstone ! Didn't know that was there. It's quite a bit more complicated than what I have here though however I'm probably missing test cases that would've failed for this. Will take a look.
There was a problem hiding this comment.
That's been updated.
| const viewModel = super.getViewModel(payload, errors) | ||
|
|
||
| // Add instruction text to the component for rendering | ||
| if (this.instructionText) { |
There was a problem hiding this comment.
we already have options available as a field, could we not use that rather than duplicating the var?
if (options.instructionText)
There was a problem hiding this comment.
Yeah good point, that been updated.
| name: ComponentDef['name'] | ||
| title: ComponentDef['title'] | ||
| schema?: Extract<ComponentDef, { schema: object }>['schema'] | ||
| schema?: Extract<ComponentDef, { schema?: unknown }>['schema'] |
There was a problem hiding this comment.
this should always be an object, why is it unknown?
There was a problem hiding this comment.
I had type issues earlier, but resolved. That's been updated.
| const eastingMin = schema?.easting?.min ?? 0 | ||
| const eastingMax = schema?.easting?.max ?? 70000 | ||
| const northingMin = schema?.northing?.min ?? 0 | ||
| const northingMax = schema?.northing?.max ?? 1300000 |
There was a problem hiding this comment.
Can we avoid the magic numbers and put 70000, 1300000 in constants? What do they mean?
There was a problem hiding this comment.
Good point, I was copying the defaults in the ticket but will update. I'm surprised Sonar never picked this up... It's usually pretty good at this sort of thing.
…troduce constants for easting/northing limits
|
4 participants
Proposed change
Jira ticket:
https://eaflood.atlassian.net/browse/DF-529
Checklist
README.mdanddocs/*(where appropriate, e.g. new features).npm run test).npm run lint).npm run format).