-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feature] Remove added risk summary and response action options #41
Conversation
…into feat/remove-added-option
@@ -104,6 +111,7 @@ export function useForm(formType: FormType, id?: Id): State { | |||
isIncidentManager: isIncidentManager, | |||
}), | |||
}); | |||
setEntityData(formData); |
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.
isn't formdata already set in setConfigurableForm ? why not reuse it?
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'm updating entityData
every time a form option is removed so i can get the entityId
using the index. when i attempted to do this using formData
, i wouldn't get the correct entityId and i would run into an error onPrimaryButtonClick
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.
changes requested, not major enough to block merge/ Can be addressed in v2.
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.
thanks @deeonwuli ! Only one important comment: validate also the formState when something is deleted
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.
thanks @deeonwuli !!
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.
Approved !
📌 References
📝 Implementation
📹 Screenshots/Screen capture
🔥 Notes to the tester
#8696j3edq