-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat(form-v2): add error handling for deleted logic fields #4440
Conversation
interface FieldLogicBadgeProps { | ||
field: FormFieldWithQuestionNo | ||
field: FormFieldWithQuestionNo | null | ||
errorType?: 'error' | 'info' | ||
errorString?: string | ||
} |
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.
'info'
is not really an errorType
, maybe 'variant'
? optionally you could also type this more strongly:
type FieldLogicBadgeProps =
| { type: 'info', field: FormFieldWithQuestionNo }
| { type: 'error', message: string }
}
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.
in fact if you look at it this way, the props required for the two cases are completely different, which is forcing us to use multiple switch
statements and ternary operators all over the place. can consider splitting this into separate components. although it will result in a couple of lines of repeated code, it will reduce the case splits required everywhere else.
arguably in future we might need more types of LogicBadge
and at some point it might make sense to refactor them into the same component, but for now imo these make more sense separately. up to you though!
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.
as discussed, 'error' | 'info'
are the default fallbacks when field
is null
. This type is not ideal but I think any other way introduces a lot of complexity into the code. For example, another possibility is that we can have another component that deals with error cases specifically (so FieldLogicBadge
only takes in a field, and ErrorLogicBadge
takes in the { variant: 'error' | 'info', message: string }
) but then the error-handling behavior is repeated everywhere you want a logic badge anyway, so it boils down to the same thing
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.
i'll change the names to variant
and message
though so its less confusing!
frontend/src/features/admin-form/create/logic/hooks/useAdminFormLogic.ts
Outdated
Show resolved
Hide resolved
...min-form/create/logic/components/LogicContent/EditLogicBlock/EditCondition/ThenShowBlock.tsx
Outdated
Show resolved
Hide resolved
...min-form/create/logic/components/LogicContent/EditLogicBlock/EditCondition/ThenShowBlock.tsx
Show resolved
Hide resolved
* Compute whether any/all fields in the show fields are deleted, then run | ||
* effect to delete fields on render and show appropriate error/infobox. | ||
*/ | ||
const showValueWatch = useWatchDependency(watch, 'show') |
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 do we need to watch
this? does the component not automatically update when formFields
updates?
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.
formFields
only contains the fields in the form, not the logics. Also, we need to useWatch here since the effect right after sets the show value again, resulting in infinite rerender.
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.
i might be wrong / there might be a better way to achieve this though!
...tures/admin-form/create/logic/components/LogicContent/InactiveLogicBlock/FieldLogicBadge.tsx
Show resolved
Hide resolved
...min-form/create/logic/components/LogicContent/EditLogicBlock/EditCondition/ThenShowBlock.tsx
Outdated
Show resolved
Hide resolved
...tures/admin-form/create/logic/components/LogicContent/InactiveLogicBlock/FieldLogicBadge.tsx
Outdated
Show resolved
Hide resolved
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.
+1 again
Problem
Builder logic breaks when a user deletes a field that was used in logic. This PR fixes that by adding error handling to the various components.
Closes #4341
Solution
LogicFieldBadge.tsx
,InactiveLogicBlock.tsx
LogicFieldBadge
to display in case the field data cannot be found.InactiveLogicBlock
provides these props to theLogicFieldBadge
component.useAdminFormLogic.ts
,LogicContent.tsx
EditConditionBlock.tsx
,ThenShowBlock.tsx
Breaking Changes
Screenshots
Screen.Recording.2022-08-01.at.12.10.12.PM.mov