-
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
MPDX-8264 Fix status and frequency #1089
Conversation
Preview branch generated at https://MPDX-8264-fix-commitmen.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 0f988fa
|
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.
Great work on this! I left a few comments, but it's looking good.
src/components/Tool/FixCommitmentInfo/FixCommitmentInfo.test.tsx
Outdated
Show resolved
Hide resolved
@@ -104,44 +104,8 @@ describe('FixCommitmentContact', () => { | |||
expect(setContactFocus).toHaveBeenCalledWith(testData.id, TabKey.Donations); | |||
}); | |||
|
|||
it('should fail validation', async () => { |
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.
Should we be removing this? Seems like it's a good test to have in place to ensure the status is filled in before the form can be submitted
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.
For this tool we actually want users to be able to change certain fields without changing all the fields. So we want them to be able to leave some fields blank. I'll remove required
from the yup.
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 approving this PR. Everything looks great. I had one issue with the useMemo, but that's it.
Description
https://jira.cru.org/secure/RapidBoard.jspa?rapidView=3&view=detail&selectedIssue=MPDX-8264&sprint=1268#
https://jira.cru.org/secure/RapidBoard.jspa?rapidView=3&view=detail&selectedIssue=MPDX-8265&sprint=1268#
Checklist: