-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Introduce Form Groups #5752
Introduce Form Groups #5752
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.
Great work! The error detection code in the tab component is much more readable.
I have the general feeling that this could be more unit tested.
const newState = Array.from(fields).reduce<FormGroupState>( | ||
(acc, field) => { | ||
const fieldState = form.getFieldState(field); | ||
let errors = acc.errors; |
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.
let errors = acc.errors || [];
and remove line 40 to 42
@@ -44,7 +53,9 @@ describe('<SaveButton />', () => { | |||
<TestContext> | |||
<ThemeProvider theme={theme}> | |||
<SaveContextProvider value={saveContextValue}> | |||
<SaveButton {...invalidButtonDomProps} /> | |||
<FormContextProvider value={formContextValue}> |
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 don't understand why this is now required and it worries me (that it's now required). this will be a gotcha for users.
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 actually don't get how those tests could have pass before. Indeed, the SaveButton
has always been retrieving the setOnSave
function from the FormContext
and should have failed
errors[field] = fieldState.error; | ||
} | ||
|
||
const newState = { |
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.
Please add a unit test for that logic
export const findTabsWithErrors = (children, errors) => { | ||
// TODO: Provide documentation link |
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.
todo
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.
Almost there!
Co-authored-by: Francois Zaninotto <[email protected]>
Introducing form groups will allow us to remove the restrictions on the
FormTab
content which could accept any components and not just inputs while still tracking whether any of their inputs are invalid.