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.

Sign up for GitHub

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

Jump to bottom

Improve i18n in return authorization views. #817

Merged
merged 1 commit into from
Apr 8, 2016
Merged

Improve i18n in return authorization views. #817

merged 1 commit into from
Apr 8, 2016

Conversation

Murph33
Copy link
Contributor

@Murph33 Murph33 commented Feb 9, 2016

Use model name and model attribute translations where logical.

This is part of an ongoing effort to improve I18n use in solidus as discussed in #735.

@jhawthorn jhawthorn added the i18n label Feb 12, 2016
@Murph33 Murph33 force-pushed the improve_I18n_return_authorizations branch from 032ce27 to 22d1855 Compare February 12, 2016 21:37
@jhawthorn jhawthorn added i18n and removed i18n labels Feb 17, 2016
cbrunsdon
cbrunsdon reviewed Feb 18, 2016
View reviewed changes
backend/app/views/spree/admin/return_authorizations/_form.html.erb
@@ -76,13 +76,13 @@
<% end %>

<%= f.field_container :stock_location do %>
<%= f.label :stock_location, Spree.t(:stock_location) %>
<%= f.label :stock_location_id %>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we generally use Spree::StockLocation.human_name for these, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. This wasn't merged in yet but it would be taken care of https://github.com/solidusio/solidus/pull/876/files#diff-3 if it's merged first or need to be rebased to take care of. I did this wrong in a number of places (every place in that PR) but since some were merged and some not I thought it made the most sense to handle it all in one PR and rebase as necessary.

Either way I can handle this change now instead of pushing something into master just to immediately change it.

@Murph33 Murph33 force-pushed the improve_I18n_return_authorizations branch from 22d1855 to e92d5ff Compare February 18, 2016 20:51
@Murph33
Copy link
Contributor Author

Murph33 commented Feb 18, 2016

So I tossed those other translations into new translations on model attributes that don't have corresponding column in the DB but if we want to do that I might need to take another sweep through the forms to be consistent because I'm pretty sure there are still some Spree.t( ) translations in table headers.

@tvdeyen
Copy link
Member

tvdeyen commented Feb 19, 2016

Spree.t translations are not evil per se. If the key is specific enough, it's ok to use them.

We should avoid them as much as possible, but "Ausnahmen machen die Regel", as Germans say.

@Murph33 Murph33 force-pushed the improve_I18n_return_authorizations branch from e92d5ff to bdda19d Compare March 10, 2016 18:14
mamhoff
mamhoff reviewed Mar 31, 2016
View reviewed changes
core/config/locales/en.yml
@@ -69,6 +69,7 @@ en:
spree/line_item:
description: Item Description
name: Name
pre_tax_amount: Pre-Tax Refund Amount
Copy link
Contributor

Choose a reason for hiding this comment

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

This column does not exist anymore, and is not used for refunds anymore either (see #941 and #706).

Besides, it's use further up is for a return item form, not a line item form.

@mamhoff
Copy link
Contributor

mamhoff commented Mar 31, 2016

I'd say let's go on a case-by-case basis here. Aside from the comments I made regarding amount vs. pre_tax_amount, this PR is 👍 by me.

@Murph33 Murph33 force-pushed the improve_I18n_return_authorizations branch from bdda19d to c03343b Compare March 31, 2016 18:43
@mamhoff
Copy link
Contributor

mamhoff commented Mar 31, 2016

👍

@mamhoff
Copy link
Contributor

mamhoff commented Apr 6, 2016

@Murph33 Would you mind rebasing this on current master?

@Murph33
Improve i18n in return authorization views.
2119b80 
Use model name and model attribute translations where logical.

Change-Id: I8a56f3fa9aa158f4d2be205fce1808aa65728e9a
@Murph33 Murph33 force-pushed the improve_I18n_return_authorizations branch from c03343b to 2119b80 Compare April 6, 2016 16:51
@mamhoff mamhoff merged commit 9c659ac into solidusio:master Apr 8, 2016
@mamhoff
Copy link
Contributor

mamhoff commented Apr 8, 2016

Thanks!

@Murph33 Murph33 deleted the improve_I18n_return_authorizations branch April 8, 2016 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Murph33 @tvdeyen @mamhoff @cbrunsdon @jhawthorn