-
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
DAS-7055, DAS-7064 Improve feedback errors for missing fields in report forms #496
Conversation
@@ -30,7 +32,7 @@ const filterOutEnumErrors = (errors, schema) => errors // filter out enum-based | |||
}, schema); | |||
} | |||
|
|||
return !!match && !match.enum; | |||
return !!match || match?.enum?.length; |
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 was causing for some required fields to not show an error
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.
Looks really good! Could we get a couple unit tests for ErrorMessages.js
to document and ensure its functionality?
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.
Getting close! Let's give the tests another round first.
src/ReportForm/ReportFormBody.js
Outdated
@@ -8,14 +8,16 @@ import styles from './styles.module.scss'; | |||
|
|||
const additionalMetaSchemas = [draft4JsonSchema]; | |||
|
|||
const getLinearErrorPropTree = (errorProperty) => { | |||
return errorProperty.replace(/'|\.properties|\[|\]|\.enumNames|\.enum/g, '.') |
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.
Maybe we can create a constant for the regex, using an explicit name that explains the purpose of it. This way it would be easier to know what we are trying to replace here.
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.
💯
@@ -70,7 +70,8 @@ describe('Error messages', () => { | |||
expect(expandedAccordion).toBeTruthy(); | |||
|
|||
userEvent.click(detailsButton); | |||
expect(screen.getByRole('menuitem', { expanded: false })).toBeTruthy(); | |||
notExpandedAccordion = screen.getByRole('menuitem', { expanded: false }); | |||
expect(notExpandedAccordion).toBeTruthy(); |
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.
Much better! 🎉
https://allenai.atlassian.net/browse/DAS-7055
Evidence