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

Removes call to config option through explicit classname #1273

Conversation

chrp
Copy link

@chrp chrp commented Oct 7, 2018

ActiveAdmin relies on a subclassing FormBuilder. For remotely related reasons I also need to set the skipped_columns option only for that subclass but the explicit call to Formtastic::FormBuilder.skipped_columns here prevents me from doing that.

General question: Should I be able to copy the config when subclassing from FormBuilder to be able to alter it's behaviour? Or should there be only one global config.

For my very specific problem: Removing the explicit reference to the class actually works quite fine (except that a future dev might have trouble finding out where skipped_columns came from)

@mikz
Copy link
Contributor

mikz commented Oct 8, 2018

This looks like a very reasonable change. The change that introduced it (d3814b4) did not count that the inputs are actually included to the form builder anyway.

Could you cover it with a test?

@chrp
Copy link
Author

chrp commented Oct 8, 2018

Happy to cover it with a test of cause. Does it make sense to subclass FormBuilder and test that I can change the setting for that subclass? Or what should the test cover in your eyes?

@mikz
Copy link
Contributor

mikz commented Oct 8, 2018

The test could cover that a subclass can have different skipped columns than the parent class (FormBuilder). So yeah, creating a subclass and setting the skipped_columns property, then verifying it does not render those columns.

@mikz mikz merged commit caeac77 into formtastic:master Oct 16, 2018
@mikz
Copy link
Contributor

mikz commented Oct 16, 2018

Thank you!

@chrp chrp deleted the chrp_configurable_skipped_columns_when_subclassing_form_builder branch October 17, 2018 12:19
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