-
Notifications
You must be signed in to change notification settings - Fork 87
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
fix: add validation for myinfo child name field #7875
Conversation
@@ -322,33 +315,28 @@ const ChildrenBody = ({ | |||
<FormLabel gridArea="formlabel">Child</FormLabel> | |||
<Flex align="stretch" alignItems="stretch" justify="space-between"> | |||
<Box flexGrow={10}> | |||
<FormControl key={field.id} isRequired isInvalid={!!childNameError}> | |||
<FormControl key={field.id} isRequired isInvalid={!!error?.[0]}> |
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.
Sudden use of hardcoded index access (i.e., [0]
) looks suspicious. Should it be the index
of the current child instead?
i.e., Child Component has two rows, the validation error is on 2nd child instead, would this error?.[0]
pick up the error on the correct child?
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.
hi ken, seems like the hardcoding of the index where 0th element refers to the name field is a deterministic behaviour since a while ago. (see attached ss)
exploring further, the index of the error message is fixed. if an error does not exist, then it will be an empty element. for eg, this screenshot shows a case when only the 5th field has an error and the others do not - the rest of the fields still keep the same index but are empty.
hence, the above Child Component has two rows, the validation error is on 2nd child instead, would this error?.[0] pick up the error on the correct child?
is guaranteed to not occur
was considering adding a const NAME_INDEX = 0
and using it but not sure if it will add more clarity
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.
Yeah I think const NAME_INDEX = 0
gives it more clarity, since I was also confused why it is always the first index. Explicitly calling it out makes sense.
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.
ill add it in then
Continuation of PR 7874
Tests
myinfo child fields are properly validated as required by the FE.