Add I18n support for `:placeholder` HTML option is passed to form fields #16438

Merged
merged 1 commit into from Aug 14, 2014

Conversation

Projects
None yet
5 participants
@agrobbin
Contributor

agrobbin commented Aug 9, 2014

This is my first real contribution to the Rails source, so any and all feedback is appreciated!

I looked at #3674 (which appears to have been the implementation of #768), but since that was back in 2012, and there didn't appear to be any actual opposition to this addition, I'm giving this a shot against the latest master.

This adds full I18n support for the :placeholder HTML5 input/textarea attribute. It tries to mimic the label I18n support almost exactly, first checking helpers.placeholder, then falling back to object.class.human_attribute_name, then @method_name.humanize. It only attempts to check for I18n values if you pass the :placeholder option, as requested in the original PR.

Basic usage:

<%= f.text_field :post, :body, placeholder: true %>

will look up the I18n value in this order:

helpers.placeholders.post.body
activerecord.attributes.post.body
attributes.body

I thought about extracting the common pieces from this and the Label tag, but thought getting feedback on the first pass would make more sense.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 9, 2014

Member

Thank you for the pull request.

I like the proposal but I have mixed feelings about it. I know Simple Form already supports this feature and I'm sure we don't want to the default Rails form builder to become Simple Form.

@jeremy WDYT?

Member

rafaelfranca commented Aug 9, 2014

Thank you for the pull request.

I like the proposal but I have mixed feelings about it. I know Simple Form already supports this feature and I'm sure we don't want to the default Rails form builder to become Simple Form.

@jeremy WDYT?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Aug 9, 2014

Member

Great first contribution, @agrobbin !

I'm not sure whether the form helper should be responsible for placeholder i18n either. It's already simple to placeholder: t(...), and that accounts for having different placeholders depending on the context, rather than global defaults for all object.attr.

That said, we do i18n on many other form helpers so it's disappointing that this one spot people rely on doesn't work like the others. Translating symbol values of placeholder: :... could be a nice way to address that.

Member

jeremy commented Aug 9, 2014

Great first contribution, @agrobbin !

I'm not sure whether the form helper should be responsible for placeholder i18n either. It's already simple to placeholder: t(...), and that accounts for having different placeholders depending on the context, rather than global defaults for all object.attr.

That said, we do i18n on many other form helpers so it's disappointing that this one spot people rely on doesn't work like the others. Translating symbol values of placeholder: :... could be a nice way to address that.

@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Aug 9, 2014

Contributor

The thought here was that a frequent pattern has become to replace labels with :placeholder attributes, providing something that takes less space on mobile sites while still giving context to the user about what to enter into the form field. That was my rationale behind having it mirror the label I18n functionality.

@jeremy your suggestion about translating symbol values, if I'm understanding you correctly, feels like it would be very redundant:

<%= f.text_field :post, :body, placeholder: :body %>

The main issue I've been dealing with in one of the new applications I've been working on is that I've had to continuously include something like this in my forms:

<%= f.text_field :post, :body, placeholder: Post.human_attribute_name(:body) %>

That just feels gross :)

Contributor

agrobbin commented Aug 9, 2014

The thought here was that a frequent pattern has become to replace labels with :placeholder attributes, providing something that takes less space on mobile sites while still giving context to the user about what to enter into the form field. That was my rationale behind having it mirror the label I18n functionality.

@jeremy your suggestion about translating symbol values, if I'm understanding you correctly, feels like it would be very redundant:

<%= f.text_field :post, :body, placeholder: :body %>

The main issue I've been dealing with in one of the new applications I've been working on is that I've had to continuously include something like this in my forms:

<%= f.text_field :post, :body, placeholder: Post.human_attribute_name(:body) %>

That just feels gross :)

@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Aug 13, 2014

Contributor

Is there something I should be doing to update this new feature, or is this request still just something that you are contemplating @rafaelfranca @jeremy?

Contributor

agrobbin commented Aug 13, 2014

Is there something I should be doing to update this new feature, or is this request still just something that you are contemplating @rafaelfranca @jeremy?

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Aug 13, 2014

Member

I'm positive on it, since it matches label i18n.

The thing that puzzles me with this and label i18n:

helpers.placeholders.post.body
activerecord.attributes.post.body
attributes.body

Labels and placeholders usually aren't boilerplate text that's identical for every form. I expected to see this try a translation key scoped by the AV template first.

Given that label doesn't do this either, fine to match its behavior IMO. But I'd find this behavior frustrating!

Member

jeremy commented Aug 13, 2014

I'm positive on it, since it matches label i18n.

The thing that puzzles me with this and label i18n:

helpers.placeholders.post.body
activerecord.attributes.post.body
attributes.body

Labels and placeholders usually aren't boilerplate text that's identical for every form. I expected to see this try a translation key scoped by the AV template first.

Given that label doesn't do this either, fine to match its behavior IMO. But I'd find this behavior frustrating!

@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Aug 13, 2014

Contributor

Labels and placeholders usually aren't boilerplate text that's identical for every form. I expected to see this try a translation key scoped by the AV template first.

@jeremy I have a PR in the works that adds some other logic to label and can take a swing at this if you want!

Contributor

agrobbin commented Aug 13, 2014

Labels and placeholders usually aren't boilerplate text that's identical for every form. I expected to see this try a translation key scoped by the AV template first.

@jeremy I have a PR in the works that adds some other logic to label and can take a swing at this if you want!

@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Aug 13, 2014

Contributor

I've rebased this off of the latest master and added a CHANGELOG entry to Action View. Should be good to go!

Contributor

agrobbin commented Aug 13, 2014

I've rebased this off of the latest master and added a CHANGELOG entry to Action View. Should be good to go!

jeremy added a commit that referenced this pull request Aug 14, 2014

Merge pull request #16438 from agrobbin/input-placeholder-i18n
Add I18n support for `:placeholder` HTML option is passed to form fields

@jeremy jeremy merged commit dfc3f88 into rails:master Aug 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Aug 22, 2014

Member

@agrobbin This looks like the same implementation as label, but that's a bit different - it has both value and content.

For a placeholder, a string value needs to remain a literal string. A symbol value could be treated as an i18n key.

cc @georgeclaghorn

Member

jeremy commented Aug 22, 2014

@agrobbin This looks like the same implementation as label, but that's a bit different - it has both value and content.

For a placeholder, a string value needs to remain a literal string. A symbol value could be treated as an i18n key.

cc @georgeclaghorn

@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Aug 22, 2014

Contributor

@jeremy 👍 I will issue a new PR with that change and regression tests included.

Contributor

agrobbin commented Aug 22, 2014

@jeremy 👍 I will issue a new PR with that change and regression tests included.

@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Aug 22, 2014

Contributor

@jeremy this appears to already work as you state with the current implementation of the code. This test appears to pass no problem:

def test_text_field_placeholder_with_string_value
  with_locale :placeholder do
    assert_dom_equal('<input id="post_cost" name="post[cost]" placeholder="How much?" type="text" />', text_field(:post, :cost, placeholder: "How much?"))
  end
end

Am I missing something?

Contributor

agrobbin commented Aug 22, 2014

@jeremy this appears to already work as you state with the current implementation of the code. This test appears to pass no problem:

def test_text_field_placeholder_with_string_value
  with_locale :placeholder do
    assert_dom_equal('<input id="post_cost" name="post[cost]" placeholder="How much?" type="text" />', text_field(:post, :cost, placeholder: "How much?"))
  end
end

Am I missing something?

@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Aug 22, 2014

Contributor

Ah, figured it out! It was working in tests because of how human_attribute_name works by humanizeing the supplied attribute. I've now made it explicit within the Placeholderablemodule to just stop if the passed value is a String. Issued a new PR at #16639.

Contributor

agrobbin commented Aug 22, 2014

Ah, figured it out! It was working in tests because of how human_attribute_name works by humanizeing the supplied attribute. I've now made it explicit within the Placeholderablemodule to just stop if the passed value is a String. Issued a new PR at #16639.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Aug 24, 2014

Member

Great! Thank you @agrobbin ❤️

Member

jeremy commented Aug 24, 2014

Great! Thank you @agrobbin ❤️

@Nowaker

This comment has been minimized.

Show comment
Hide comment
@Nowaker

Nowaker Sep 7, 2014

Nice feature. ...But how to omit it? I'm not able to pass an arbitrary value for the placeholder now. I can pass autofocus, class and other HTML attributes just fine, except for placeholder. This is not good. It would be OK if it used default wisely, and showed my content if there's no i18n entry.

= f.text_field :hostname, placeholder: 'Enter Machine Hostname (e.g. myapp.company.com)'
<input id="machine_hostname" name="machine[hostname]" placeholder="Com)" type="text">

Nowaker commented Sep 7, 2014

Nice feature. ...But how to omit it? I'm not able to pass an arbitrary value for the placeholder now. I can pass autofocus, class and other HTML attributes just fine, except for placeholder. This is not good. It would be OK if it used default wisely, and showed my content if there's no i18n entry.

= f.text_field :hostname, placeholder: 'Enter Machine Hostname (e.g. myapp.company.com)'
<input id="machine_hostname" name="machine[hostname]" placeholder="Com)" type="text">
@agrobbin

This comment has been minimized.

Show comment
Hide comment
@agrobbin

agrobbin Sep 7, 2014

Contributor

@Nowaker this was an oversight of this PR, and was fixed as part of #16639. The next 4.2 beta should bring that functionality back!

Contributor

agrobbin commented Sep 7, 2014

@Nowaker this was an oversight of this PR, and was fixed as part of #16639. The next 4.2 beta should bring that functionality back!

@Nowaker

This comment has been minimized.

Show comment
Hide comment
@Nowaker

Nowaker Sep 7, 2014

@agrobbin Cool, thanks!

Nowaker commented Sep 7, 2014

@agrobbin Cool, thanks!

@frankhereford

This comment has been minimized.

Show comment
Hide comment
@frankhereford

frankhereford Jan 11, 2016

Thank you! This is super helpful!

Thank you! This is super helpful!

ebihara99999 added a commit to ebihara99999/expert-todo that referenced this pull request Sep 30, 2017

ebihara99999 added a commit to ebihara99999/expert-todo that referenced this pull request Sep 30, 2017

ebihara99999 added a commit to ebihara99999/expert-todo that referenced this pull request Sep 30, 2017

ebihara99999 added a commit to ebihara99999/expert-todo that referenced this pull request Nov 9, 2017

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