This project is archived and is in readonly mode.

#745 ✓resolved
JackC

Form label should use I18n

Reported by JackC | August 2nd, 2008 @ 04:36 AM | in 2.3.6

Form labels should look in active_record.human_attribute_names namespace in the I18n system for the correct label text.

f.label(:name) should equal f.label(:name, t(:name))

Comments and changes to this ticket

  • Glenn Powell

    Glenn Powell August 4th, 2008 @ 02:01 PM

    I think this boils down to String.humanize (and thus Inflector.humanize) should first check I18n to see if the argument can be located there, and then proceed with it's normal functionality.

  • iain

    iain August 10th, 2008 @ 03:49 PM

    You'd probably want something like: :'active_record.human_attribute_names.product.name'

    Problem is that String.humanize doesn't know which model is used. It just knows 'name'.

    So the only place to do this is in the form.label method, because it's the only place where both the model and the attribute is known.

  • Glenn Powell

    Glenn Powell August 10th, 2008 @ 06:51 PM

    Yes, after looking more into it, I came to this conclusion as well. So we can only hope that the form.label will perform a check in i18n similar to the error_messages helper.

  • iain

    iain August 24th, 2008 @ 11:52 AM

    Fixed it a bit nicer in the i18n project:

    http://i18n.lighthouseapp.com/pr...

  • Jeremy Kemper

    Jeremy Kemper November 22nd, 2008 @ 08:59 PM

    • State changed from “new” to “open”
    • Milestone cleared.
  • Pratik

    Pratik January 7th, 2009 @ 10:30 PM

    • Assigned user set to “Sven Fuchs”
  • Niels Ganser

    Niels Ganser January 21st, 2009 @ 04:04 PM

    Additional to Iains linked patch, I would also suggest running titleize on the string returnd by I18n.

    The reasoning behind that being that in your translation files you probably have your attribute names all lowercase as that's how you'd want see them in your AR error messages. For a label though, a titlecased version makes more sense.

    Attached is my patch suggestion (incl. test) agains 2-2-stable.

  • Sven Fuchs

    Sven Fuchs January 21st, 2009 @ 10:52 PM

    Regarding titleizing the translation: I can see that the user might want to titleize the attribute name for certain locales. But I don't think we can safely assume this is true for all locales.

    So, do we need a way to control this? Putting an option for this into the translation data would seem extremely ugly to me. Any suggestions?

  • Niels Ganser

    Niels Ganser January 22nd, 2009 @ 02:25 AM

    If we know for sure that other languages have other "titlecasing" rules, I would suggest putting the corresponding logic right into the titleize method and storing the rules in the locale's .yml.

    Sven, you are of course right though. Putting such an option or rule set into the translation data is not exactly beautiful or intuitive but on the other hand, there's already e.g. support.array.skip_last_comma in there which serves a similar purpose and frankly I can't think of a better place to store that stuff.

    Although I would find a distinction between localisation (date formats, titlecasing rules, currency precision stuff, etc) and translation (strings!) files helpful, this ticket is certainly not the right place to discuss something like this..

  • DHH

    DHH February 7th, 2009 @ 01:15 PM

    • Milestone set to “2.x”
  • Niels Ganser

    Niels Ganser March 5th, 2009 @ 09:33 PM

    After reading up on the issue of capitalization a little (e.g. Wikipedia: "The full rules of capitalization for English are complicated."), I think we should go ahead implementing the very crude version of just uppercasing the first letter of human_attribute_name.

    Unless someone can come up with a central repository of capitalization rules for different locales, I just don't see it being worth the effort to collect that kind of information. Whenever someone isn't quite happy with just having the first character capitalised, one can always overwrite the string in the locale .ymls, no?

    What do you think, Sven?

  • José Valim

    José Valim March 6th, 2009 @ 10:33 AM

    human_attribute_name when used without I18n calls humanize on your attribute name, which uppercases the first letter. So you should have capitalized names on your I18n files as well.

    In other words, I think that no further processing should be done after calling human_attribute_name, because it should return a string which already has the first letter uppercased.

    If someone needs lowercased words in AR error messages, we should provide a patch for it, but not adopt the wrong behavior here.

  • iain

    iain March 6th, 2009 @ 01:25 PM

    Agrees with José. Titleize is a good option when no translation has been specified, but when you specify another string in your locale I want Rails to respect my version and not to change it afterwards.

    Expample: in Dutch, the letter "ij" is uppercased to IJ (e.g. IJmuiden). If it would always titleize, I could never have a form without spelling errors, which would be very bad.

  • Niels Ganser

    Niels Ganser March 6th, 2009 @ 02:04 PM

    I agree with you two about not blatantly overriding/manipulating a localised string already provided by the user.

    However, I think one must distinguish between three basic use cases of human_attribute_name:

    1. At the beginning of a sentence, the way they are currently used in validation error messages in ActiveRecord
    2. As some sort of title or label, e.g. in forms, as proposed here
    3. As a normal part of any sentence

    Maybe I am overcomplicating here but an attribute name as simple as first_name would be expected to be represented in three different ways according to the use cases above:

    1. First name
    2. First Name
    3. first name

    So in my mind just assuming that activerecord.attributes.model.attribute will be set to whatever is right is a bit of a cop-out as "whatever is right" depends on the context it is used in. I think that ideally it should just be set to "first name" with utility methods such as titleize being locale-aware and taking care of the necessary transformations.

    Obviously though, this is beyond the scope of this ticket. So I would propose going ahead with Iain's patch for the time being. Everyone then has to take care of fancy titlecasing themselves but it is certainly better than simply humanising column names as we do now.

  • José Valim

    José Valim March 6th, 2009 @ 03:36 PM

    Niels,

    In the first and second cases I use: "First name". I guess then it changes from developer to developer. :)

    I'm attaching Iain's patch as form_label_simple.diff, if everyone agree, this should be the one applied.

  • Dmitry Polushkin

    Dmitry Polushkin July 27th, 2009 @ 05:25 AM

    • Tag changed from “patch, tiny” to “2.3.4, patch, tiny”

    Any news?

  • José Valim

    José Valim August 14th, 2009 @ 05:37 PM

    • Milestone changed from “2.x” to “2.3.4”
  • Sven Fuchs

    Sven Fuchs August 26th, 2009 @ 01:22 PM

  • Priit Tamboom

    Priit Tamboom September 2nd, 2009 @ 12:44 PM

    José Valim: By the way, when nested forms is used, label method don't get clean "object" to klassify from it but object would be something like "post[comments_attributes][0]" in some blog app where post model has_many :comments with accepts_nested_attributes_for :comments.

  • Jeremy Kemper

    Jeremy Kemper September 11th, 2009 @ 11:04 PM

    • Milestone changed from “2.3.4” to “2.3.6”

    [milestone:id#50064 bulk edit command]

  • Júlio Santos Monteiro

    Júlio Santos Monteiro October 6th, 2009 @ 05:24 PM

    +1, this is very important to internacionalizated apps.

  • ronin-75041 (at lighthouseapp)

    ronin-75041 (at lighthouseapp) November 5th, 2009 @ 01:53 PM

    • Tag changed from “2.3.4, patch, tiny” to “2.3.4, form_helper, i18n, patch, tiny”
  • Carsten Gehling

    Carsten Gehling December 4th, 2009 @ 07:36 AM

    I have a IMHO better idea for this.

    Most of the time I want the form label to display a slightly different string than just the field name. For this purpose I have created an extra scope inside the YML activerecord structure, named "labels":

    activerecord:
    ... models:

    customer: "Kunden"

    attributes:

    name: "Name"
address: "Adresse"
zip: "PLZ"

    labels:

    name: "Name (Vorname und Name)"
address: "Adresse (Strasse und Hausnummer)"
zip: "PLZ (immer 5-stellig)"

    This makes it much more simple to control how the content of label should be presented.

    I have made a simple rewrite of the to_label_tag method, to include this:

    module ActionView
    module Helpers

    class InstanceTag
  def to_label_tag(text = nil, options = {})
    options = options.stringify_keys
    tag_value = options.delete("value")
    name_and_id = options.dup
    name_and_id["id"] = name_and_id["for"]
    add_default_name_and_id_for_value(tag_value, name_and_id)
    options.delete("index")
    options["for"] ||= name_and_id["id"]

    content = text.blank? ? nil : text.to_s
    content ||= I18n.t("activerecord.labels.#{object_name}.#{method_name}", :raise => true) rescue nil
    content ||= method_name.humanize
    label_tag(name_and_id["id"], content, options)
  end
end

    end

    end

    I hope you'll find this interesting

  • Carsten Gehling

    Carsten Gehling December 4th, 2009 @ 07:42 AM

    Bloody hell, something happened to the formatting. :-) Here's the meat - hopefully more readable:

    YML:

    activerecord:
  ...
  models:
    

    


    

    customer: "Kunden"

    


    
  attributes:


    name: "Name"
address: "Adresse"
zip: "PLZ"

    


    
  labels:


    name: "Name (Vorname und Name)"
address: "Adresse (Strasse und Hausnummer)"
zip: &quot;PLZ (immer 5-stellig)&quot;</code>

    The code:

    module ActionView
  module Helpers
    

    


    

    class InstanceTag
  def to_label_tag(text = nil, options = {})
    options = options.stringify_keys
    tag_value = options.delete(&quot;value&quot;)
    name_and_id = options.dup
    name_and_id[&quot;id&quot;] = name_and_id[&quot;for&quot;]
    add_default_name_and_id_for_value(tag_value, name_and_id)
    options.delete(&quot;index&quot;)
    options[&quot;for&quot;] ||= name_and_id[&quot;id&quot;]

    content = text.blank? ? nil : text.to_s
    content ||= I18n.t(&quot;activerecord.labels.#{object_name}.#{method_name}&quot;, :raise =&gt; true) rescue nil
    content ||= method_name.humanize

    label_tag(name_and_id[&quot;id&quot;], content, options)
  end
end

    


    
  end
end

    Sorry for that.

    • Carsten
  • José Valim

    José Valim December 4th, 2009 @ 09:27 AM

    Yes, Carsten, some time ago we had this discussion on I18n mailing list and we agreed in something quite similar! We agreed in:

    • First, look in views.labels.model.attribute
    • Then use Model.human_attribute_name
    • And finally use attribute.humanize

    Could you provide a patch with tests according to the contributors guide? https://rails.lighthouseapp.com/projects/8994/sending-patches

    Thanks!

  • Carsten Gehling

    Carsten Gehling December 4th, 2009 @ 09:35 AM

    Ahh thanks - I wasn't aware of that.

    views.labels.model.attribute sounds perfect indeed (along with the other defaults as well of course)

    I will submit a patch ASAP

    • Carsten
  • Carsten Gehling

    Carsten Gehling December 22nd, 2009 @ 06:21 AM

    José,

    I have been coding and testing on this patch now, but one thing puzzles me: Hasn't Model.human_attribute_name been deprecated and removed from Rails 2.3.x?

    Carsten

  • José Valim

    José Valim December 22nd, 2009 @ 07:14 AM

    No, it has not. At least, it should not.

  • Carsten Gehling

    Carsten Gehling December 22nd, 2009 @ 09:09 AM

    Here's the patch with tests and modified documentation.

    Please note, that this is my first contribution to the core framework. I've followed the guidelines to the best of my knowledge, but if I missed something, please let me know. :-)

    Carsten

  • Carsten Gehling

    Carsten Gehling December 22nd, 2009 @ 09:27 AM

    Added a little more examples to the label helper doc.

    Carsten

  • José Valim

    José Valim December 22nd, 2009 @ 09:28 AM

    @Carsten, congrats, the patch and tests look great!

    I will ask just a small change though, instead of raising an error and rescuing on I18n call (which is slow), please do this instead:

    content = if text.blank?
  i18n_label = I18n.t("views.labels.#{object_name}.#{method_name}", :default => "")
  i18n_label if i18n_label.present?
else
  text.to_s
end

content ||= object.class.human_attribute_name(method_name) if object
content ||= method_name.humanize
  • Carsten Gehling

    Carsten Gehling December 22nd, 2009 @ 10:00 AM

    Yes of course - bad coding habit of mine :-)

    Here you go.

    Carsten

  • José Valim

    José Valim December 22nd, 2009 @ 09:07 PM

    Thanks Carsten, finally, the patch you gave me contains three commits. Could you please squash them and provide a patch with just one commit?

  • Sven Fuchs

    Sven Fuchs December 23rd, 2009 @ 04:53 PM

    Hey guys,

    this looks good to me, except that:

    • I don't get why the "".present? thing is better than using :raise => true / rescue nil because this is exactly what :raise is for. But maybe that's a matter of taste.
    • The patch includes the translations "Total cost" and "Write entire text here" into actionpack/lib/action_view/locale/en.yml Instead it should set these translations to the backend in the test setup or test.
    • (Why is it that there's an extra local variable content created instead of just using the existing text argument?)

    José, thanks for the ping!

  • Carsten Gehling

    Carsten Gehling December 23rd, 2009 @ 09:33 PM

    @Sven:

    1) José argued that "".present? should execute faster than raising and catching an exception. I personally have no benchmarks proving either case. And I certainly have no special preference for one over the other. :-)

    2) Whoops! Sorry I thought that the en.yml was a test-specific fixture-file. I'll fix it asap

    3) Well it just follows the original code-pattern which read:

    content = (text.blank? ? nil : text.to_s) || method_name.humanize
    label_tag(name_and_id["id"], content, options)

    I'll fix this no. 2 (and any of the others if you want it) on saturday and send a new patch.

    @José: I am - ahem - quite new to Git. Is there a simple way to mash these commits down to one in the patch, or should I checkout the Rails code and start over?

    Happy Christmas all of you! See you in a couple of days. :-)

    • Carsten
  • José Valim

    José Valim December 23rd, 2009 @ 10:35 PM

    Carsten, there are probably several ways to do this in Git, but they way I usually do is to execute "git reset HEAD^1" until all the commits were reverted (or "git reset HEAD^3"). The nice thing is that this still leaves the code in your working branch, so you are able to work on it and then provide a brand new patch. :)

  • Carsten Gehling

    Carsten Gehling December 29th, 2009 @ 12:14 PM

    Mery christmas eveybody!

    I've modified the patch so that:

    • Everything is now in one single commit (thanks José)
    • The changes to the en.yml locale file has been removed. Instead I create a separate locale as part of the form_helper_test setup

    @Sven: If you would rather prefer me using the :raise => true and catch pattern instead of .present?, then please let me knowæ. Otherwise the patch should be complete.

    Carsten

  • Sven Fuchs

    Sven Fuchs December 29th, 2009 @ 01:07 PM

    Thanks Carsten, I'm cool with present?, too :)

    So, +1

  • Paul Campbell

    Paul Campbell December 29th, 2009 @ 09:22 PM

    +1, been waiting on this for ages!

  • Carsten Gehling

    Carsten Gehling December 30th, 2009 @ 09:57 AM

    Thanks Paul :-)

    José? You're cool?

  • José Valim

    José Valim December 30th, 2009 @ 09:59 AM

    Oh, yeah! Thanks Carsten!

  • Carsten Gehling

    Carsten Gehling December 30th, 2009 @ 10:00 AM

    • Tag changed from “2.3.4, form_helper, i18n, patch, tiny” to “2.3.4, form_helper, i18n, patch, tiny, verified”
  • Repository

    Repository January 2nd, 2010 @ 09:28 PM

  • Repository

    Repository January 2nd, 2010 @ 11:20 PM

  • José Valim

    José Valim January 2nd, 2010 @ 11:24 PM

    • State changed from “open” to “resolved”

    Committed, thanks!

  • Sven Fuchs

    Sven Fuchs January 3rd, 2010 @ 02:09 AM

    big applause :) thanks josé!

  • Carsten Gehling

    Carsten Gehling January 3rd, 2010 @ 03:00 PM

    Thanks Sven :-) Got a nice warm feeling inside - since this is my first patch.

    Makes me want to do some more with this nice reception. When I look at https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/milesto... are these all the tickets that needs to be resolved, before 2.3.6 is released?

    Carsten

  • José Valim

    José Valim January 3rd, 2010 @ 03:09 PM

    Carsten, those tickets in the milestone are not official, since I think anyone can assign any milestone. In other words, feel free to tackle any issue you find on lighthouse, not specifically those. :)

    And you are more than welcome to participate in the next bugmash: http://railsbridge.org/news_items/9

  • Carsten Gehling

    Carsten Gehling January 3rd, 2010 @ 06:12 PM

    I'm on holiday that weekend, but I'll just mash some bugs on my own accord :-)

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages