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

reset should set initial model #140

Closed
macrozone opened this issue Nov 28, 2016 · 5 comments · Fixed by #145
Closed

reset should set initial model #140

macrozone opened this issue Nov 28, 2016 · 5 comments · Fixed by #145
Assignees
Labels
Type: Bug Bug reports and their fixes

Comments

@macrozone
Copy link
Contributor

macrozone commented Nov 28, 2016

I tried reset() for the first time.

It added a button to the QuickForm and called this.reset();.

It thought it would reset the state to the given props.model on the form, but instead, all fields were emptied. Shouldn't it reset to the model instead? if no model is given, then yes, it should empty all fields.

Edit:

it's on this line:

https://github.com/vazco/uniforms/blob/master/packages/uniforms/src/components/forms/AutoForm.js#L65

i think it should set it back to this.props.model instead of {},

Setting it back to props.model would be more logical to me, but this would be a breaking change. Opinions?

@radekmie radekmie added the Type: Bug Bug reports and their fixes label Nov 28, 2016
@radekmie radekmie self-assigned this Nov 28, 2016
@radekmie
Copy link
Contributor

So you've tried it on AutoForm - there's no internal model in QuickForm :)

Yes, it is a breaking change, but every bugfix is a breaking change because something is working differently now, doesn't it? This should behave exactly the same as the initial state in the constructor, so as you've said - this.props.model in both model and modelSync.

Would you like to submit a PR for it?

@radekmie
Copy link
Contributor

radekmie commented Dec 1, 2016

@macrozone?

@macrozone
Copy link
Contributor Author

@radekmie: Yes, I can do this in the next days. But generally, you do agree with this change?

@radekmie
Copy link
Contributor

radekmie commented Dec 1, 2016

Yes, I do agree. If you are using it in a fully automatic way (you have no model), then we are not changing anything. If you are using it as a default model, then the new behaviour is the desired one. At last, if you are using it as a managed form, then it's still OK because it'll keep the model from props.

@radekmie
Copy link
Contributor

Available in v1.8.0.

@radekmie radekmie moved this to Closed in Open Source Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants