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

Make FormHelper extension required_label behave the same as label for i18n #2954

Closed
wants to merge 5 commits into from

Conversation

johanb
Copy link
Contributor

@johanb johanb commented Apr 20, 2015

For your consideration:

Refinery has a required_label form helper method. This intended behavior of required_label seems to be to append a " *" to the content and add a required class. Currently however it changes Rails default fallback behavior of the label method for forms.

From the docs:

Returns a label tag tailored for labeling an input field for a specified attribute (identified by method) on an object assigned to the template (identified by object). The text of label will default to the attribute name unless a translation is found in the current I18n locale (through helpers.label..) or you specify it explicitly. Additional options on the label tag can be passed as a hash with options. These options will be tagged onto the HTML as an HTML element attribute as in the example shown, except for the :value option, which is designed to target labels for #radio_button tags (where the value is used in the ID of the input tag).

emphasis mine

Refinery's current implementation does not respect this behavior and jumps straight to the AR method described in the rails docs:

I've made a PR which changes this behavior to mimic the Rails fallback behavior without changing the arity of the method definition of required_label (for compatibility sake).

While changing this behavior I couldn't find existing specs, is this correct or did I look in the wrong places ?

TODO

  • Spec this
  • Discuss possibility of changing the arity to be compatible with the label method

Update

I'd actually prefer to remove this entire thing from Refinery, it solves something so trivial. I'd be happy to change this PR to that.. Seems it's not used in this: #2922

@parndt
Copy link
Member

parndt commented Apr 21, 2015

@johanb removing it sounds great.

@johanb
Copy link
Contributor Author

johanb commented Apr 22, 2015

@parndt I've removed it.

This is however used here: https://github.com/refinery/refinerycms-inquiries/blob/master/app/views/refinery/inquiries/inquiries/_form.html.erb you want me to whip up a PR for the extensions hosted under "refinery" that depend on this to ?

@parndt
Copy link
Member

parndt commented Apr 22, 2015

@johanb that'd be lovely, thanks!

@bricesanchez bricesanchez added this to the 3.0 milestone May 2, 2015
@johanb johanb closed this May 7, 2015
@johanb johanb deleted the default_rails_i18n_behavior branch May 7, 2015 07:53
@johanb johanb restored the default_rails_i18n_behavior branch May 7, 2015 07:53
@johanb johanb reopened this May 7, 2015
@johanb
Copy link
Contributor Author

johanb commented May 7, 2015

Accidentally removed my branch, which closed the PR. Sorry

@parndt parndt closed this in b4b20cd May 7, 2015
@parndt
Copy link
Member

parndt commented May 7, 2015

Merged into a single commit b4b20cd, thanks!

@ghoppe
Copy link
Contributor

ghoppe commented Jun 19, 2015

This breaks rails g refinery:form

@johanb
Copy link
Contributor Author

johanb commented Jun 23, 2015

Missed that @ghoppe, will investigate. Thanks for reporting it.

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