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

Add i18n for include_blank #616

Closed
wants to merge 5 commits into from
Closed

Add i18n for include_blank #616

wants to merge 5 commits into from

Conversation

haines
Copy link
Contributor

@haines haines commented Jul 11, 2012

For a collection select input, :include_blank => "string" gives <option value="">string</option>.

This patch allows the user to specify a locale file like

en:
  simple_form:
    include_blank:
      model:
        attribute: "not applicable"

in order to automatically translate :include_blank => true into :include_blank => "not applicable"

@rafaelfranca
Copy link
Collaborator

Hey mate! This pull request seems good but I want to propose this feature to be added directly in Rails.

See #176 (comment)

What do you think?

@haines
Copy link
Contributor Author

haines commented Jul 12, 2012

Yep that makes sense to me. I'll submit one over there!

@haines haines closed this Jul 12, 2012
@rafaelfranca
Copy link
Collaborator

Lets leave this one open until the Rails one is accepted.

@rafaelfranca rafaelfranca reopened this Jul 12, 2012
@haines
Copy link
Contributor Author

haines commented Jul 12, 2012

Ok having looked at how this works in Rails, I've changed my mind.

Doing the translations in Rails means that they apply to all selects.

  • :prompt should be universally translated because (a) the default is "Please select:", so changing it is key to internationalization and (b) even in English, it can be useful to change the default across an entire application (say if you prefer "Select:" to "Please select:" or whatever).
  • :include_blank, on the other hand, defaults to a language-neutral empty string, so it is not necessary to translate this when internationalizing. Further, it doesn't make sense to change it universally - the representation of "no value" is generally context-dependent (it might mean "not applicable" or just "empty").

The big advantage to translating both :prompt and :include_blank within SimpleForm would be that the text would be customisable on a per-attribute setting. (:prompt should of course fall back to helpers.select.prompt).

en:
  simple_form:
    include_blank:
      person:
        age: "Prefer not to say"
        car: "No car"

    prompt:
      car:
        make: "Choose your car's make:"
        color: "Paint your car:"

I think this is better than having to specify these options every time you make an input for that attribute.

What do you reckon?

@rafaelfranca
Copy link
Collaborator

I'm fine with doing this in SimpleForm.

@josevalim @carlosantoniodasilva thoughts?

@carlosantoniodasilva
Copy link
Member

I'm ok to add this to SimpleForm itself, no problem.

So in short, we're going to always lookup for include_blank, since SimpleForm always defaults to that, and lookup for prompt when the user says prompt: true (which disables include_blank), is that correct?

@haines can you please also add tests to ensure that when giving false to both include_blank and prompt, it's not going to appear with an existing translation?

Thanks.

@haines
Copy link
Contributor Author

haines commented Sep 9, 2012

@carlosantoniodasilva Yep that's pretty much right. The user can still specify include_blank: "Something" or prompt: "Something else" and that will get used instead of looking up the translation.

@carlosantoniodasilva
Copy link
Member

Alright. I'm just a little bit worried about extra translation lookups we'll be doing for free now, for those who use include_blank with no text attached. Other than that, I think we're fine to go.

@josevalim
Copy link
Contributor

I don't think it makes sense to translate when include_blank is set to true. I think it works this way in Rails by accident, but :include_blank should never actually add a text, people should be using :prompt for that. That said, I think this should work as:

  1. The default in Simple Form is to set include blank to true unless include_blank or prompt are explicitly set (this already works today)
  2. If a user sets prompt explicitly to true, we should translate it

In other words, I believe we just need to change this pull request to translate only prompt values. If we do that, everything else should work as expected.

@haines
Copy link
Contributor Author

haines commented Sep 9, 2012

I think it does make sense for include_blank to add text because it behaves differently to prompt.

prompt not shown when the field already has a value. Therefore for a nullable column, it's important to use include_blank instead, otherwise it's impossible to set the value back to null.

In those cases it can sometimes be better to have include_blank: "Not applicable" or "None" or something. It helps the user understand what setting the value to null actually means, which doesn't happen if you just have a completely blank option. I think this is probably why it works this way in Rails.

prompt, on the other hand, usually only makes sense on required fields, and should generally be something like "Please select:" or "Choose an option:" - unlike include_blank, it's not a valid option itself.

@haines
Copy link
Contributor Author

haines commented Jan 10, 2013

Any more thoughts on this, guys?

@rafaelfranca
Copy link
Collaborator

I still not sure, but I think this feature should be opt-in. So users that doesn't want it will not have to do a i18n lookup

@haines
Copy link
Contributor Author

haines commented Jan 31, 2013

Yeah, makes sense! How about this:

If include_blank and prompt are both omitted, SimpleForm just sets include_blank to true, and doesn't do a i18n lookup (i.e. exactly how it works at the moment). This preserves the current behaviour for most cases.

If include_blank is explicitly set to true, then do the lookup. This would be considered opting in to the translation, because you don't need to specify it otherwise.

That would avoid doing lots of extra unexpected lookups.

@josevalim
Copy link
Contributor

This sounds like an easy way out but I find it confusing because you end up having three values: nil (i.e. the default), true (translation) and false (skip it). I would prefer include_blank: :from_i18n or something similar to trigger the I18n lookup. Even better, if we are going with the Symbol approach, we could even do include_blank: :gender_prompt or something like that and allow devs to choose a key instead of an attribute based lookup.

@haines
Copy link
Contributor Author

haines commented Jan 31, 2013

Yeah, could work - I like the idea of being able to choose a key. I'll knock something up tomorrow.

@nashby
Copy link
Collaborator

nashby commented Feb 1, 2013

but how include_blank: :gender_prompt differs from include_blank: t(:gender_prompt)?

@haines
Copy link
Contributor Author

haines commented Feb 1, 2013

I think it would be equivalent to include_blank: t(:gender_prompt, scope: :simple_form)

Or perhaps even better, it could be include_blank: :gender or prompt: :gender, which would be t(:gender, scope: "simple_form.include_blanks") and t(:gender, scope: "simple_form.prompts") respectively.

@haines
Copy link
Contributor Author

haines commented Feb 6, 2013

Ok, I have changed it so that all lookups are opt-in. The current behaviour is maintained:

  • include_blank: true or omitted generates <option value="" />
  • include_blank: false suppresses the <option value="" />
  • include_blank: "Text" generates <option value="">Text</option>

The new behaviour is:

  • include_blank: :translate uses Inputs::Base#translate("include_blanks", true).
  • include_blank: :t is a shortcut for include_blank: :translate
  • include_blank: :some_key is equivalent to I18n.t(:"simple_form.include_blanks.some_key")

Similar options apply for :prompt as well, using the i18n namespace simple_form.prompts.

@defkode
Copy link

defkode commented Feb 12, 2013

+1. It would be great to have it

@simonc
Copy link

simonc commented Jun 3, 2013

👍

1 similar comment
@mumualex
Copy link

mumualex commented Aug 9, 2013

+1

@josevalim
Copy link
Contributor

I think we should get rid of the include_blank: :translate shortcuts with the special lookup. If you want a default blank, just make it include_blank: :default with no special lookup rules.

@ahacking
Copy link

So its been over a year and still no I18n placeholder support for selects??? WTF!

This is an extremely basic feature and according to the current docs one would believe it is supported. The documentation makes no mention that i18n placeholders are not supported for selects even though they are for other fields. I've just wasted an hour tracing and debugging something that turns out to be a silent limitation of simple_form. These kinds of WTF moments make me wonder what all the added complexity in "simple form" is about. I've had grief with simple form handling of select options (eg passing extra data attributes on options), rails does it and simple_form with all its complexity doesn't.

Thank god I'm moving away from convoluted server rendered form nonsense, and far away from year long bike-sheds with no outcomes.

@dreamfall
Copy link
Contributor

@ahacking feel free to add this feature right now.

@ahacking
Copy link

@dreamfall Why? So we can have another year of bikeshed and nothing making it into master? As I said I have since migrated away from server side rendering and the need for simple form and its many unwanted surprises and deviations from what the documentation would have you believe.

@josevalim
Copy link
Contributor

@ahacking ❤️ 💚 💙 💛 💜

@rafaelfranca
Copy link
Collaborator

@ahacking 💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙

@sony-phoenix-dev
Copy link

👍

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

Successfully merging this pull request may close these issues.