Skip to content

Add form options for each step#175

Closed
fullpipe wants to merge 5 commits intocraue:masterfrom
fullpipe:step_form_options
Closed

Add form options for each step#175
fullpipe wants to merge 5 commits intocraue:masterfrom
fullpipe:step_form_options

Conversation

@fullpipe
Copy link
Contributor

@fullpipe fullpipe commented May 8, 2015

Sorry for looooooong response.
Rebased. Test added.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.14%) to 99.24% when pulling 73a9c56 on fullpipe:step_form_options into 4d14115 on craue:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.14%) to 99.25% when pulling 5df513f on fullpipe:step_form_options into 4d14115 on craue:master.

@craue
Copy link
Owner

craue commented May 11, 2015

I totally forgot about your original PR already. 😄 I'm going to close #172 in favor of this one because it's much more powerful.

I'd prefer calling them "form options" rather than just "options" to clarify what they are, i.e. please rename options to form_options, $options to $formOptions, and setOptions/getOptions to setFormOptions/getFormOptions.

Form/Step.php Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the type hint or the is_array check can be removed. Having both is useless. I'm tending slightly into removing the type hint to be consistent with the type handling of the other setters.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.75%) to 98.64% when pulling 179f6af on fullpipe:step_form_options into 4d14115 on craue:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.75%) to 98.64% when pulling 179f6af on fullpipe:step_form_options into 4d14115 on craue:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.39% when pulling 179f6af on fullpipe:step_form_options into 4d14115 on craue:master.

Form/Step.php Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$formOptions please. The string for the step config should be 'form_options' like you did, but not the property.

@craue
Copy link
Owner

craue commented May 15, 2015

I'm seeing one issue with this now: What should happen if one option is defined in all three possible places, e.g. validation_groups? Should they be merged (somehow) or should they be overwritten in a specific order? And I guess both approaches make sense (depending on the use case). Thus I'll not close #172 but merge it as well to provide an easy way to just add validation groups, while this PR will simply overwrite options (including the generated step-specific validation group).

@craue
Copy link
Owner

craue commented May 15, 2015

@fullpipe: But you just need to fix those two remaining commit comments. I'll rebase, squash, merge then.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.14%) to 99.25% when pulling 4ad628f on fullpipe:step_form_options into 4d14115 on craue:master.

@craue
Copy link
Owner

craue commented Jun 1, 2015

Thank you, @fullpipe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants