Skip to content
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

Fixed form.onValues() based on setState() #13

Merged
merged 1 commit into from Jul 24, 2016

Conversation

@artkravchenko
Copy link
Contributor

artkravchenko commented Jul 4, 2016

Related issue: #12

Please review and verify these changes.

@artkravchenko

This comment has been minimized.

Copy link
Contributor Author

artkravchenko commented Jul 11, 2016

@theadam Any ideas?

@theadam

This comment has been minimized.

Copy link
Owner

theadam commented Jul 11, 2016

The code looks good, I was trying to think of a way to add a test to ensure this case is covered in the future. Could you take a stab at that?

@theadam

This comment has been minimized.

Copy link
Owner

theadam commented Jul 20, 2016

Hows it looking, @Oopscurity?

@artkravchenko artkravchenko force-pushed the artkravchenko:fix/onvalues-validate branch from e5c3ee7 to 431fba9 Jul 20, 2016
@artkravchenko

This comment has been minimized.

Copy link
Contributor Author

artkravchenko commented Jul 20, 2016

@theadam Seems like the problem has been solved. Verify please.

@theadam

This comment has been minimized.

Copy link
Owner

theadam commented Jul 20, 2016

@Oopscurity That is correct, this code looks right, but an added test case will help prevent this from happening again in the future! Thanks!

@artkravchenko

This comment has been minimized.

Copy link
Contributor Author

artkravchenko commented Jul 20, 2016

@theadam The test that you're talking about was added. :)

@theadam

This comment has been minimized.

Copy link
Owner

theadam commented Jul 20, 2016

hahah @Oopscurity I am very sorry, I see it now :) I am wondering if it makes sense to not necessarily touch the fields when onValues is called. Is there a case where someone might want to change the underlying value without showing the error. What do you think?

src/form.js Outdated
@@ -95,9 +95,12 @@ export default function form({fields, validate = () => ({})}) {
forceValidate: () => this.touch(fields),
values: () => this.state.values,
onValues: values => {
if (!this.props.value) {
this.setState({ values });
this.setValues(values);

This comment has been minimized.

Copy link
@theadam

theadam Jul 20, 2016

Owner

I am not sure we want to set these values if there is a props.value being passed here. If there is, it will overwrite whats there. It might be better to retain the !this.props.value check. In this case, the onChange event should be fired to the parent thats controlling the value prop and allow them to pass the proper values down. (When new values are passed, setValues is called, so if the parent is just passing the same values after an onChange this will have the same effect).

src/form.js Outdated
if (!this.props.value) {
this.setState({ values });
this.setValues(values);
if (typeof values === 'object') {

This comment has been minimized.

Copy link
@theadam

theadam Jul 20, 2016

Owner

Maybe instead of this, we forceValidate is changed to accept an array of fields, but default to all the fields:
forceValidate: (fieldsToValidate = fields) => this.touch(fieldsToValidate),.

With this the user can update the values of the fields without being forced to mark them as touched. If the user wants both, they can just call

onValues(changes);
forceValidate(Object.keys(changes));

themselves. This seems like it would be more flexible to me, but I would like to hear your opinion!

@artkravchenko artkravchenko force-pushed the artkravchenko:fix/onvalues-validate branch from 431fba9 to aa26bfc Jul 23, 2016
@artkravchenko

This comment has been minimized.

Copy link
Contributor Author

artkravchenko commented Jul 23, 2016

@theadam I've just changed the structure of test cases and fixed the problems you had considered. Validate please.

@theadam

This comment has been minimized.

Copy link
Owner

theadam commented Jul 24, 2016

Thanks for taking the time, looks good!

@theadam theadam merged commit 74e9efd into theadam:master Jul 24, 2016
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.