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

[WIP][Reference][Form Types] Update "radio" form type #3433

Merged
merged 3 commits into from
Jan 21, 2014
Merged

[WIP][Reference][Form Types] Update "radio" form type #3433

merged 3 commits into from
Jan 21, 2014

Conversation

bicpi
Copy link
Contributor

@bicpi bicpi commented Jan 6, 2014

Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed tickets #3410

radio's parent is checkbox, not form.

I need some help regarding the task value option should be removed from this type:
Should it really be removed? It's because I'm able to set it and it is used in the HTML output. I'm retrieving a boolean value only when I access the submitted data via the form framework but I have also access to the raw, custom value via e.g. $request->request->get('my_checkbox'). So maybe it should be moved to the inherited options instead for the radio field? If yes, the wording would need an update for the value option and if no, why keep it for the checkbox field?

@@ -27,7 +27,7 @@ If you want to have a Boolean field, use :doc:`checkbox </reference/forms/types/
| | - `error_mapping`_ |
| | - `mapped`_ |
+-------------+---------------------------------------------------------------------+
| Parent type | :doc:`form </reference/forms/types/form>` |
| Parent type | :doc:`form </reference/forms/types/checkbox>` |
Copy link
Member

Choose a reason for hiding this comment

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

form should also be changed to checkbox

@wouterj
Copy link
Member

wouterj commented Jan 6, 2014

You're correct about the value type. In the Inherited Options sections, add a new thing at first which starts with: " These options inherit from the :doc:checkbox </reference/forms/types/checkbox> type:" and then document the value option

@bicpi
Copy link
Contributor Author

bicpi commented Jan 7, 2014

@wouterj The value option has it's own file now and I tweaked the content a little bit to also fit with radios.

@wouterj
Copy link
Member

wouterj commented Jan 7, 2014

👍

@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2014

Can you rebase this?

@bicpi
Copy link
Contributor Author

bicpi commented Jan 10, 2014

@xabbuh Ups, that was a rebase on master by mistake. Mh, good question how to roll this back...

@bicpi
Copy link
Contributor Author

bicpi commented Jan 10, 2014

@xabbuh Now the rebase should be okay.

@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2014

👍 great @bicpi, this should make life easier for @weaverryan since #3416 was merged in the meantime

@wouterj
Copy link
Member

wouterj commented Jan 10, 2014

yes, it's succesfully rebased!

@weaverryan
Copy link
Member

Great work Philipp! This is very subtle, tough to get perfectly. Thanks for your attention on it.

weaverryan added a commit that referenced this pull request Jan 21, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

[WIP][Reference][Form Types] Update "radio" form type

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3+
| Fixed tickets | #3410

`radio`'s parent is `checkbox`, not `form`.

I need some help regarding the task *value option should be removed from this type*:
Should it really be removed? It's because I'm able to set it and it is used in the HTML output. I'm retrieving a boolean value only when I access the submitted data via the form framework but I have also access to the raw, custom value via e.g. `$request->request->get('my_checkbox')`. So maybe it should be moved to the `inherited options` instead for the `radio` field? If yes, the wording would need an update for the `value` option and if no, why keep it for the `checkbox` field?

Commits
-------

322b21e Update&Outsource "value" option & update references
be47b90 Fix parent type doc reference
22b3d0c [Reference][Form Types] Update "radio" form type
@weaverryan weaverryan merged commit 322b21e into symfony:2.3 Jan 21, 2014
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.

4 participants