-
Notifications
You must be signed in to change notification settings - Fork 123
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
VAGOV-TEAM-90731: add ombinfo prop #31788
Conversation
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.
First off, beautiful code. This is really well written, organized, etc.
I wonder, though, if this code is implementing the cleanest approach. It feels a bit odd to me to treat the loading (and storage via Redux) of the OMB info in a manner that is isolated from the rest of the form data that is loaded and subsequently converted into a formConfig
object.
This code snippet, specifically, feels like it should already be doing what we want to do:
// src/applications/form-renderer/utils/formConfig.js (before)
const formConfig = createFormConfig(form);
dispatch(formLoadingSucceeded(formConfig));
It feels odd to have to also call a dispatch
for OMB-specific info:
// src/applications/form-renderer/utils/formConfig.js (proposed change)
const formConfig = createFormConfig(form);
dispatch(ombInfoLoaded(form.ombInfo));
dispatch(formLoadingSucceeded(formConfig));
It'd be great if the formConfig
object would already encapsulate the needed OMB info so that we wouldn't need to introduce it separately on the Redux store. I think we can do that:
// src/applications/form-renderer/utils/formConfig.js (createFormConfig function)
return {
...
introduction: props => (
<IntroductionPage {...props} ombInfo={ombInfo} />
),
...
}
If we need to abstract this behavior for reuse, we could create a higher-order component:
const withProps = (WrappedComponent, additionalProps) => {
return (props) => <WrappedComponent {...props} {...additionalProps} />;
};
I'm certainly open to other thoughts on this. Perhaps we can discuss.
formLoad: reducer.formLoad, | ||
ombInfo: reducer.ombInfo, |
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 feels odd to me. See more general discussion.
TL;DR it seems like loading OMB info should be part of loading the other form data.
dispatch(ombInfoLoaded(form.ombInfo)); | ||
dispatch(formLoadingSucceeded(formConfig)); |
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.
Again, see more general discussion. It feels a bit odd to have the OMB data treated separately from the rest of the form data.
|
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.
Nice work!
I approved, but I did add two small comments after doing so. I don't consider them significant enough to block approval, but they are worth considering, perhaps. |
d8333bb
to
c23cdea
Compare
Are you removing, renaming or moving a folder in this PR?
Summary
ombInfo
section created by VATEAM-90733: Normalize OMB info fields content-build#2252 from fetched form data and inserts it into theIntroductionPage
component.Related issue(s)
Testing done
ombInfo
action and reducer.IntroductionPage
container.form-load
reducer and theApp
container.Screenshots
Local host rendering a mock form with dynamic OMB Info data:
What areas of the site does it impact?
This PR only touches the
form-renderer
application, which is not yet live.Acceptance criteria
Quality Assurance & Testing
Error Handling
Authentication
Requested Feedback
Due to our use of
createRoutesWithSaveInProgress
to create our child routes, I could not easily pass along data outside offormConfig
to theIntroductionPage
component as props. Instead, I loaded it into our Redux store and pulled from that. This is relevant to future work, as we will eventually want to customize bothIntroductionPage
andConfirmationPage
more thoroughly on a per-form basis. Neither of these components required access to the Redux store before this PR, so continuing on in this direction will make both components more complex.