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

Use include_blank value as option label #17064

Merged
merged 1 commit into from
Oct 25, 2014
Merged

Use include_blank value as option label #17064

merged 1 commit into from
Oct 25, 2014

Conversation

frenkel
Copy link
Contributor

@frenkel frenkel commented Sep 26, 2014

Update select_tag to reflect documentation and behave the same as form builder select. If the value of include_blank is not boolean true, use that value as the option label.

include_blank = ''
end

option_tags = content_tag(:option, '', :value => include_blank).safe_concat(option_tags)
Copy link
Member

Choose a reason for hiding this comment

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

The value should be always empty string. Only the text that can be the include_blank value

@frenkel
Copy link
Contributor Author

frenkel commented Sep 26, 2014

@rafaelfranca Thanks for pointing that out. I fixed it.

@rafaelfranca
Copy link
Member

Great! Now we will need a test to assert this behaviour and avoid regressions.

@frenkel
Copy link
Contributor Author

frenkel commented Sep 26, 2014

I've added a test for it.

@frenkel
Copy link
Contributor Author

frenkel commented Oct 13, 2014

@rafaelfranca Is there still something wrong? Or can this be merged?

@arthurnn
Copy link
Member

LGTM, before merging, can you squash all your commits in 1. so would make it easier to track back in history.

thanks

Update select_tag to reflect documentation and behave the same as form builder select. If the value of include_blank is not boolean true, use that value as the option label.
@frenkel
Copy link
Contributor Author

frenkel commented Oct 17, 2014

Great! In my branch I've force-pushed a merged commit. The Github interface doesn't seem to recognize this though.

@frenkel
Copy link
Contributor Author

frenkel commented Oct 21, 2014

@arthurnn @rafaelfranca Is this correct now? Can it be merged?

rafaelfranca added a commit that referenced this pull request Oct 25, 2014
@rafaelfranca rafaelfranca merged commit 0afbe74 into rails:master Oct 25, 2014
rafaelfranca added a commit that referenced this pull request Oct 25, 2014
rafaelfranca added a commit that referenced this pull request Oct 25, 2014
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.

None yet

3 participants