-
Notifications
You must be signed in to change notification settings - Fork 43
First iteration of dynamic form section looping #1387
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.
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
base: main
Are you sure you want to change the base?
Conversation
| /** | ||
| * RepeatingSectionSummaryPageController is for pages summarising a set of sections | ||
| */ | ||
| export class RepeatingSectionSummaryPageController extends PageController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will have to coordinate with all UKHSA teams using this type of form if you're replacing this page controller completely or rewrite this as a non-breaking change. (Might be fine - just a warning!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No teams are using this controller other than mine :)
| getPartialState!: RepeatingFieldPageController["getPartialState"]; | ||
| options!: RepeatingFieldPageController["options"]; | ||
| removeAtIndex!: RepeatingFieldPageController["removeAtIndex"]; | ||
| hideRowTitles!: RepeatingFieldPageController["hideRowTitles"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assume just shortcutting types here for ease whilst you prototype?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all of these types will be needed, so some will be removed
| isRepeatingFieldPageController = true; | ||
| isSamePageDisplayMode: boolean; | ||
| isSeparateDisplayMode: boolean; | ||
| isRepeatingSection: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RepeatingFieldPageController is just intended to repeat a single field multiple times (i.e. 1 page which has 1 field that repeats). Has this option been added to RepeatingFieldPageController to allow recursive repeating fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution utilises existing RepeatingFieldPageController functionality - see/test out the attached form to get an idea of how it all comes together
| delete request.payload['crumb']; | ||
| const sectionName = this.section.name; | ||
| const newObj = {}; | ||
| newObj[sectionName + 'Container'] = [request.payload]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is + 'Container' needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See/test out the attached form to get an idea of how it all comes together
| makePostRouteHandler() { | ||
| return async (request: HapiRequest, h: HapiResponseToolkit) => { | ||
| const modifyUpdate = () => { | ||
| delete request.payload['crumb']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the csrf cookie needed to be deleted? I would also just be careful about using delete in general, since it would mutate request.payload directly. It might be safe in this instance but can cause non-obvious side-effects with how js handles objects (by reference)
If you need the payload without the csrf cookie - do something like this
const { crumb, ...payload } = request;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed so that the payload object is cloned before the crumb is removed
This is an incomplete solution for allowing repeating sections of data, not just repeating single fields. To test this, use the attached JSON form:
looping-textfield.json