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.

Sign up for GitHub

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

Jump to bottom

Add a hidden field on the collection_radio_buttons #18303

Merged
merged 2 commits into from
Sep 26, 2015
Merged

Add a hidden field on the collection_radio_buttons #18303

merged 2 commits into from
Sep 26, 2015

Conversation

maurogeorge
Copy link
Contributor

The same behavior that occurs on file_field(#17947) occurs with the collection_radio_buttons.

@rafaelfranca rafaelfranca self-assigned this Jan 2, 2015
rafaelfranca
rafaelfranca reviewed Jan 2, 2015
View reviewed changes
actionview/lib/action_view/helpers/tags/collection_radio_buttons.rb
@@ -14,7 +14,8 @@ def radio_button(extra_html_options={})
end

def render(&block)
render_collection do |item, value, text, default_html_options|
options = @options.stringify_keys
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify the implementation of this method and collection_check_boxes moving it render_collection. Also I don't think we need to stringify in this case.

@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from 86a22b6 to 131cc3b Compare January 6, 2015 20:59
@maurogeorge
Copy link
Contributor Author

@rafaelfranca I updated the code following your suggestions.

About collection_check_box it dont have the test of include_hidden as string, I added the missing test, now both works with the string. I added the stringfy to fix this.

@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from 131cc3b to 7b571d6 Compare January 6, 2015 21:03
@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch 2 times, most recently from 3a42911 to 8831820 Compare January 20, 2015 12:32
@maurogeorge
Copy link
Contributor Author

@rafaelfranca when you have some time could you please review 😄

@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from 8831820 to 682c70a Compare March 28, 2015 14:05
@maurogeorge
Copy link
Contributor Author

@rafaelfranca fixed the conflicts, when you have some time could you please review

@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from 682c70a to d37149b Compare May 7, 2015 22:05
@maurogeorge
Copy link
Contributor Author

@rafaelfranca please review when you have some time 😄

@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from d37149b to f2d158d Compare June 13, 2015 15:32
@maurogeorge
Copy link
Contributor Author

@rafaelfranca I rebased again to fix the conflicts, please review when you have some time.Is there anything I can do to help? @carlosantoniodasilva maybe you can review this 😄

claudiob
claudiob reviewed Sep 3, 2015
View reviewed changes
actionview/lib/action_view/helpers/tags/collection_helpers.rb
end
end

def hidden_field #:nodoc:
Copy link
Member

Choose a reason for hiding this comment

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

Hello @maurogeorge – I'm 👍 with this PR 😀

Since you added def hidden_field, could the one already defined in collection_check_boxes.rb be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @claudiob great to hear this 😄

You are right I can remove the mentioned hidden_field. I updated the code and rebased.

@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from f2d158d to 08e956e Compare September 3, 2015 21:51
rafaelfranca
rafaelfranca reviewed Sep 3, 2015
View reviewed changes
actionview/lib/action_view/helpers/tags/collection_check_boxes.rb
@template_object.check_box(@object_name, @method_name, html_options, @value, nil)
end
end

def render(&block)
rendered_collection = render_collection do |item, value, text, default_html_options|
default_html_options[:multiple] = true
Copy link
Member

Choose a reason for hiding this comment

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

Multiple is not being handled anymore with your refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rafaelfranca I moved the multiple options to the check_box it is covered by the tests, if I remove this line I got some broken tests

@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from 08e956e to 696f619 Compare September 4, 2015 10:16
claudiob
claudiob reviewed Sep 4, 2015
View reviewed changes
actionview/CHANGELOG.md

Example:

* Add a `hidden_field` on the `collection_radio_buttons` to avoid raise a error
Copy link
Member

Choose a reason for hiding this comment

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

There is something strange in this change… it should only show your new lines, not replace the existing one.
Did you accidentally edit the next CHANGELOG message too?

If you review this, also fix avoid raise to avoid raising 😉

@maurogeorge
Add a hidden field on the collection_radio_buttons
491013e 
This will avoid a error be raised when the only input on the form is the
`collection_radio_buttons`.
@maurogeorge maurogeorge force-pushed the collection_radio_buttons_hidden_field branch from 696f619 to 578bc4b Compare September 24, 2015 23:00
@maurogeorge
Copy link
Contributor Author

@claudiob @rafaelfranca thanks a lot for the review ❤️ 😍
Sorry for the delay, I was involved with other things 😢
I updated the code, please take a look when you have some time 😄

rafaelfranca added a commit that referenced this pull request Sep 26, 2015
@rafaelfranca
Merge pull request #18303 from maurogeorge/collection_radio_buttons_h…
ae1f295 
…idden_field

Add a hidden field on the collection_radio_buttons
@rafaelfranca rafaelfranca merged commit ae1f295 into rails:master Sep 26, 2015
@meinac meinac mentioned this pull request Sep 27, 2015
Merged
@maurogeorge
Copy link
Contributor Author

Thanks for the merge ❤️

@kyamaguchi kyamaguchi mentioned this pull request Nov 9, 2015
Closed
@coorasse coorasse mentioned this pull request Jul 12, 2018
Closed
peteryates added a commit to x-govuk/govuk-form-builder that referenced this pull request Dec 18, 2020
@peteryates
Add include_hidden argument to radios collection
ad9adfc 
Rails has had this functionality since version 5[0] so it makes sense
for it to be supported here. This change uses the same keyword,
include_hidden, as Rails so should feel familiar to users.

[0] rails/rails#18303
peteryates added a commit to x-govuk/govuk-form-builder that referenced this pull request Dec 18, 2020
@peteryates
Add include_hidden argument to radios collection
7843800 
Rails has had this functionality since version 5[0] so it makes sense
for it to be supported here. This change uses the same keyword,
include_hidden, as Rails so should feel familiar to users.

[0] rails/rails#18303
@peteryates peteryates mentioned this pull request Dec 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rafaelfranca rafaelfranca

Labels
actionview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maurogeorge @claudiob @rafaelfranca @eileencodes