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

Bugfix/nested objects b34f0a25ce0c775a29a14241b14e9bc0e47976c8 #2022

Conversation

eric-burel
Copy link
Contributor

@eric-burel eric-burel commented Jul 24, 2018

Nested form wouldn't work anymore.

This was due to the field retrieval methods (getMutableFields, getInsetableFields) using systematically this.state.schema as the current schema. But when you want the fields of a nested schema, you need to pass it explicitely. Same for createField.

value prop of the FormNested was not defined either, instead the whole currentValues object is passed.

However this fix needs a review, I am not sure what was the intent of the previous modifications on the Form component.

I also added a few tests (that does almost nothing), so that we start taking good habits :)
Unit testing is a bit complex and heavy because we need to run Meteor even for pure frontend testing, and Mocha have less features than Jest. But a few regression tests are never a bad thing.
It would also be a good idea to write a few acceptance tests on the Vulcan Starter app with Chimp to detect such issues more easily.

@SachaG
Copy link
Contributor

SachaG commented Jul 25, 2018

Thanks! btw the reason for the previous modifications was to enable adding "static" (non-mutable) fields to a form. For example the "stripe subscription" field here:

https://d3vv6lp55qjaqc.cloudfront.net/items/4300181G05232C3u433G/Screen%20Shot%202018-07-25%20at%2012.10.47.png?X-CloudApp-Visitor-Id=43642&v=9e34b559

Or you can use the same feature to force a field to always display even when it's not editable. For example maybe you have a "status" field that is only editable by admins but you'd still like to show it (although disabled) to other user groups. So you can do that now.

@SachaG SachaG merged commit c686048 into VulcanJS:devel Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants