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

Support include_blank option for select form helper #54

Merged
merged 2 commits into from
Mar 10, 2016

Conversation

davydovanton
Copy link
Member

No description provided.

@runlevel5
Copy link
Member

@davydovanton have you ever thought about the default option could have different messages? In Rails 4, there is prompt option.

@AlfonsoUceda
Copy link
Contributor

@davydovanton thanks for your PR!

I prefer prompt option because a person could add something like Select option to the first option and we should allow an empty string like include_blank does.

@davydovanton
Copy link
Member Author

@joneslee85 @AlfonsoUceda thanks for feedback 👍
Yes, I think you're right. I'll update this PR soon, thanks!

# <%=
# # ...
# values = Hash['it' => 'Italy', 'us' => 'United States']
# select :stores, values, options: { include_blank: true}
Copy link
Member

Choose a reason for hiding this comment

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

nitpick,

{ include_blank: true}
#to
{include_blank: true}

@jodosha
Copy link
Member

jodosha commented Mar 7, 2016

@davydovanton PING 😉


super(attributes) do
option(prompt_value.is_a?(String) ? prompt_value : '') if prompt_value
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like this solution, but when prompt_value is true prompt_value.to_s returns 'true' string. If you have better solution I'll happy to use it 👍

Copy link
Member

Choose a reason for hiding this comment

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

@davydovanton I think that prompt: should be used only with strings. For instance

<%=
  values = Hash['it' => 'Italy', 'us' => 'United States']
  select :stores, values, options: {prompt: 'Select a City'}
%>

or

<%=
  values = Hash['it' => 'Italy', 'us' => 'United States']
  select :stores, values, options: {prompt: ''}
%>

but not..

<%=
  values = Hash['it' => 'Italy', 'us' => 'United States']
  select :stores, values, options: {prompt: true}
%>

Does it makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm, I think you're right. thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@davydovanton
Copy link
Member Author

@jodosha pong :)
I added prompt option and fixed all considerations.

@jodosha jodosha self-assigned this Mar 7, 2016
attributes = { name: _input_name(name), id: _input_id(name) }.merge(attributes)
options = attributes.delete(:options) { {} }
attributes = { name: _input_name(name), id: _input_id(name) }.merge(attributes)
blank_value = options.delete(:include_blank)
Copy link
Member

Choose a reason for hiding this comment

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

@davydovanton include_blank: true overlaps with prompt: "". What if we remove the first? 😉

@jodosha jodosha added this to the next milestone Mar 10, 2016
@jodosha jodosha merged commit 84bc9f9 into hanami:master Mar 10, 2016
@jodosha
Copy link
Member

jodosha commented Mar 10, 2016

@davydovanton I've just merged this, by removing the include_blank and allowing only prompt. Thanks 👍

@davydovanton
Copy link
Member Author

thanks!

@davydovanton davydovanton deleted the select-include-blank branch March 10, 2016 15:14
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.

4 participants