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

Fix i18n inconsistencies in forms for associations #1192

Closed
wants to merge 1 commit into from

Conversation

cpytel
Copy link
Member

@cpytel cpytel commented Jul 20, 2018

Fixes #1105. As described in #1105, there are some inconsistencies in
the way Administrate handles labels for HasMany/BelongsTo/HasOne
associations.

  • HasMany (eg: Order#line_items)

    • Gives "line_items" as the label
    • No i18n.
  • BelongsTo (eg: Customer#country)

    • Defaults to "Country code" (similar to "Country id" in this case).
    • Allows i18n as helpers.label.customer.country_code.
  • HasOne (eg: Product#product_meta_tag)

    • Gives "Product Meta Tag".
    • No i18n.

The behavior of the BelongsTo field is most correct, and is preserved
in this commit.

The behavior of HasMany and HasOne has been improved as follows:

  • HasMany (eg: Order#line_items) now no longer overrides the text
    given to the label helper, and passes the attribute name. This results
    in a default label of "Line items" and one that can be overridden with
    i18n with a key at the standard location (helpers.label.order.line_items)
  • HasOne (eg: Product#product_meta_tag) now can be overriden with
    the key helpers.label.order.line_items.

The HasOne is rendered into a nested form, inside of a fieldset and
with a legend. The text of the "label" in this nested form is actually
in a legend tag. I considered the fact that this was not using the
label helper, whether it should be switch to a label from a legend, or
whether we should have a non-standard i18n key. In the end, I decided
that the least surprising behavior would be to customize the "label" for
that attribute at the expected label's i18n key.

@@ -20,7 +20,7 @@ and is augmented with [Selectize].
%>

<div class="field-unit__label">
<%= f.label field.attribute_key, field.attribute %>
<%= f.label field.attribute, for: "#{f.object_name}_#{field.attribute_key}" %>
Copy link
Member Author

Choose a reason for hiding this comment

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

If there is an existing Rails helper I can use to generate this label key, I think that would be nice, but I couldn't find one.

@@ -18,7 +18,7 @@ The form will be rendered as nested_from to parent relationship.

<%= f.fields_for field.attribute, field.data || field.nested_form.resource.class.new do |has_one_f| %>
<fieldset class="field-unit--nested">
<legend><%= field.nested_form.resource_name.titleize %></legend>
<legend><%= t "helpers.label.#{f.object_name}.#{field.nested_form.resource_name}", default: field.nested_form.resource_name.titleize %></legend>
Copy link
Member Author

Choose a reason for hiding this comment

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

If there is an existing Rails helper or class method I could use to generate this i18n key, that would be nice, but I looked and couldn't find one.

Copy link
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

Looks good! One minor comment.

I'm willing to ignore CI failures as they're dependency warnings that'll get fixed elsewhere.

@@ -1,4 +1,5 @@
development: &default
host: localhost
Copy link
Member

Choose a reason for hiding this comment

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

Was this necessary to get up and running? Should we include this in this PR, or elsewhere?

@cpytel
Copy link
Member Author

cpytel commented Jul 20, 2018 via email

Fixes #1105. As described in #1105, there are some inconsistencies in
the way Administrate handles labels for `HasMany`/`BelongsTo`/`HasOne`
associations.

* `HasMany` (eg: `Order#line_items`)
  - Gives "line_items" as the label
  - No i18n.

* `BelongsTo` (eg:  `Customer#country`)
  - Defaults to "Country code" (similar to "Country id" in this case).
  - Allows i18n as `helpers.label.customer.country_code`.

* `HasOne` (eg:  `Product#product_meta_tag`)
  - Gives "Product Meta Tag".
  - No i18n.

The behavior of the `BelongsTo` field is most correct, and is preserved
in this commit.

The behavior of `HasMany` and `HasOne` has been improved as follows:

* `HasMany` (eg: `Order#line_items`) now no longer overrides the text
  given to the `label` helper, and passes the attribute name. This results
  in a default label of "Line items" and one that can be overridden with
  i18n with a key at the standard location (`helpers.label.order.line_items`)
* `HasOne` (eg:  `Product#product_meta_tag`) now can be overriden with
  the key `helpers.label.order.line_items`.

The `HasOne` is rendered into a nested form, inside of a fieldset and
with a legend. The text of the "label" in this nested form is actually
in a legend tag. I considered the fact that this was not using the
`label` helper, whether it should be switch to a label from a legend, or
whether we should have a non-standard i18n key. In the end, I decided
that the least surprising behavior would be to customize the "label" for
that attribute at the expected label's i18n key.
@cpytel cpytel force-pushed the fix-associations-i18n-inconsistencies branch from bc37606 to 11a9b22 Compare July 20, 2018 15:01
@cpytel cpytel closed this Jul 20, 2018
@cpytel cpytel deleted the fix-associations-i18n-inconsistencies branch July 20, 2018 15:14
@nickcharlton nickcharlton added the i18n translations and language support label Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n translations and language support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants