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

Allow for form level configuration #406

Merged
merged 2 commits into from
Oct 8, 2018
Merged

Allow for form level configuration #406

merged 2 commits into from
Oct 8, 2018

Conversation

beghelli
Copy link
Contributor

Hi Kris, thanks for this project. Very helpful. :)

@kristijanhusak
Copy link
Owner

@beghelli how you want to add a custom configuration for a form? As a property (like you did in a test), or in some other way?

I think this PR is a bit overcomplicated for such feature. We could have a getConfig method on a Form class, that would merge the custom configuration with the configuration read from the form helper class.

That way we could avoid injecting the custom config into the helper and handle everything on the form class.

@beghelli
Copy link
Contributor Author

I need to also set field templates on a form level. I can't see this happening without allowing the form config needing injected into the for helper, but I might be overlooking something :)

@kristijanhusak
Copy link
Owner

You can update fields to pull the config from the parent form that is injected through constructor.
Basically my idea is to have the config property on each class, like you added, and to have a getConfig on the Form class, that will merge the default config from FormHelper and the config property that you set.
After that, update all calls to formHelper->getConfig to call Form->getConfig instead.

Not sure if i explained it well, but this solution does not require doing anything in the FormBuilder and FormHelper class, and avoids those conditional form helper injections.

@beghelli
Copy link
Contributor Author

Sure, I will give it a shot and update the pull request. Thanks!

@beghelli
Copy link
Contributor Author

@kristijanhusak updated the pull request with your suggestions. I did one change into the FormHelper class though, but just to avoid duplicating the logic of getting the configuration. Let me know what you think, thanks.

@kristijanhusak
Copy link
Owner

@beghelli also please update all the changes to use spaces instead of tabs so it can be consistent.

@kristijanhusak
Copy link
Owner

Thanks!

@kristijanhusak kristijanhusak merged commit 3585722 into kristijanhusak:master Oct 8, 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.

None yet

2 participants