-
Notifications
You must be signed in to change notification settings - Fork 1
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
(Review third) Update FI profile - mail api integration #283
(Review third) Update FI profile - mail api integration #283
Conversation
- [Add] Fetch Institution data - [Add] Breadcrumb to return to View page
- [Add] Review FI details
…d include in submitted DTO
…nd include in submitted DTO
…de in submitted DTO
…de in submitted DTO
…"Other" details text
…ion of defaultValues
Co-authored-by: S T <[email protected]>
- Improved population of existing form data - Temp fix for TextArea resizing issue - Implements `Clear form` functionality
…rofile-phase2-mail-api-integration
- improve typing - Improve redirection method (thanks Sigmund!)
src/pages/Filing/UpdateFinancialProfile/buildProfileFormDefaults.tsx
Outdated
Show resolved
Hide resolved
src/pages/Filing/ViewInstitutionProfile/FinancialInstitutionDetails.tsx
Outdated
Show resolved
Hide resolved
src/pages/Filing/UpdateFinancialProfile/UpdateIdentifyingInformation.tsx
Outdated
Show resolved
Hide resolved
Self-notes:
|
- Add Note that the data included reflects what the user changed
…i-profile-phase2-mail-api-integration
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.
Coming along really nicely as this is a more complicated form than others! I found a few issues.
Did not focus on Zod/TypeScript, input validation and error handling in this review.
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 went ahead and addressed the outstanding review comments, and I'm approving this for now and looking forward to the next PR! 👍
Dismissing this review while you're out of town. I addressed your comments above Sigmund, and we can chat about the load on error bug you found when you return. 👍
@billhimmelsbach Thanks for this. Will try and reproduce the problem and write about it. |
Part 3 of #222
Changes
Planned improvements
defaultValues
to determine which data was changed, so that we only send SBL Help the info they actually need to update. This may also be resolved if we can address theforwardRef
errors we've been seeing, which may fix the issue of changes to these fields not being registered inreact-hook-form
How to test this PR
console
ensure routing is enabled:setIsRoutingEnabled(true)
View institution profile
Update institution profile
console
, you should also see the data that will be sent.Notes