-
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
take feedback comment #1
take feedback comment #1
Conversation
src/yup.ts
Outdated
previous[path].types && | ||
previous[path].types[type] | ||
? [ | ||
...[].concat(previous[path].types[type]), |
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.
can we just use spread operator 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.
previous[path].types[type]
type is string | string[]
.
I think [].concat(previous[path].types[type])
is better than typeof previous[path].types[type] === 'string' ? [previous[path].types[type]] : previous[path].types[type]
👍
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 thought [...previous[path].types[type]]
is the same as ...[].concat(previous[path].types[type])
?
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 propose this instead [...(previous[path].types[type] || []), message]
to stay consistent with your code.
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.
@bluebill1049 @Vandell63 umm... The code doesn't support when previous[path].types[type]
is a string.
I think it's correct to use concat
.
const string1 = 'foo';
const string2 = 'bar';
// string
const test1 = [...string1, string2]; // ['f', 'o', 'o', 'bar']
const test2 = [...[].concat(string1), string2]; // ['foo', 'bar']
// string[]
const test3 = [...test2, string1]; // ['foo', 'bar', 'foo']
const test4 = [...[].concat(test2), string2]; // ['foo', 'bar', 'foo']
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 see that's fine by me 👍
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.
thanks for the explanation.
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.
👍
src/yup.test.ts
Outdated
`); | ||
expect(resolve.errors.age.types).toMatchInlineSnapshot(`undefined`); | ||
expect(resolve.errors.createdOn.types).toMatchInlineSnapshot(`undefined`); | ||
expect(resolve.errors.password.types).toMatchInlineSnapshot(`undefined`); |
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.
Just for 3 of them expect(resolve.errors.password.types).toBeUndefined()
finally.
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 will fix this 👍
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.
done.
src/yup.ts
Outdated
...(validateAllFieldCriteria | ||
? { | ||
types: { | ||
...((previous[path] && previous[path].types) || {}), |
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.
store in variable to avoid redundancy maybe with line 22 and 24. Not mandatory.
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.
Not mandatory too to save few bytes as @bluebill tell me before
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.
done.
src/yup.ts
Outdated
previous[path].types && | ||
previous[path].types[type] | ||
? [ | ||
...[].concat(previous[path].types[type]), |
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 propose this instead [...(previous[path].types[type] || []), message]
to stay consistent with your code.
thanks for the review @Vandell63 please merge it in 👍 |
d720aa2
into
Paul-Vandell:feature/support-multiple-errors-message
take comments react-hook-form#32