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

Support i18n key at translation of value in submit tag #26799

Merged
merged 1 commit into from
Feb 27, 2018

Conversation

deraru
Copy link
Contributor

@deraru deraru commented Oct 16, 2016

Summary

Currently submit tag value translation does not support i18n key style
locale key.
It confuses me a bit because many other components support i18n key
style locale key.
I added i18n key style locale key support to submit tag.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @chancancode (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@rafaelfranca
Copy link
Member

Could you explain better what is not supported now? For what I could see by the code i18n is supported

@deraru
Copy link
Contributor Author

deraru commented Oct 16, 2016

I try to write code example.

class RichBlog::PoorPost << ActiveRecord::Base
end
en:
  activerecord:
    models:
      rich_blog/poor_post: blog post
    attributes:
      rich_blog/poor_post:
        title: post title
  helpers:
    submit:
      rich_blog_poor_post:
        create: Post my post

activerecord.models and activerecord.attributes (and more) keys allow rich_blog/poor_post.
But helpers.submit key does not allow rich_blog/poor_post.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

Thanks! It make sense to me. Could you add a test case and a CHANGELOG entry?

@@ -1879,6 +1879,7 @@ def submit_default_value
end

defaults = []
defaults << :"helpers.submit.#{object.model_name.i18n_key}.#{key}" if object.respond_to?(:model_name)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a new lookup can we add the :"helpers.submit.#{object.model_name.i18n_key}.#{key}" if it object responds to model_name and defaults << :"helpers.submit.#{object_name}.#{key}" if it doesn't?

Copy link
Contributor Author

@deraru deraru Oct 25, 2016

Choose a reason for hiding this comment

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

What should I think about specification of :as option?
If :as option is given to form_for method, is it prior than actual model name in translation?
For example,

class Blog::Post << ActiveRecord::Base
end
form_for @blog_post, as: :my_post do |f|
  f.submit
end
en:
  helpers:
    submit:
      "blog/post":
        create: Post blog post
      my_post:
        create: Post my post

Then which value is expected Post blog post or Post my post?

@@ -1879,6 +1879,7 @@ def submit_default_value
end

defaults = []
defaults << :"helpers.submit.#{object.model_name.i18n_key}.#{key}" if object.respond_to?(:model_name)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a new lookup can we add the :"helpers.submit.#{object.model_name.i18n_key}.#{key}" if it object responds to model_name and defaults << :"helpers.submit.#{object_name}.#{key}" if it doesn't?

@deraru
Copy link
Contributor Author

deraru commented Oct 16, 2016

Could you add a test case and a CHANGELOG entry?

Got it!

@deraru deraru force-pushed the support-i18n-key-in-submit-tag branch 3 times, most recently from 8a93d72 to 3bdd0f5 Compare October 24, 2016 07:17
@m5o
Copy link

m5o commented Oct 25, 2016

i18n-debug is very useful to debug/visualize which translations are being looked up by Rails

Just in case you didn't know this handy helper 🤓

@deraru
Copy link
Contributor Author

deraru commented Oct 26, 2016

@deraru
Copy link
Contributor Author

deraru commented Dec 2, 2016

@rafaelfranca Any update on this?

@deraru deraru force-pushed the support-i18n-key-in-submit-tag branch from 3bdd0f5 to ce9661b Compare January 1, 2017 06:12
@@ -2248,7 +2248,12 @@ def submit_default_value
end

defaults = []
defaults << :"helpers.submit.#{object_name}.#{key}"
# Object is a model and it is not overwritten by as and scope option.
if object.respond_to?(:model_name) && object_name.to_s == model.downcase
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider :as and :scope option is the top priority for i18n key.

@@ -917,7 +920,7 @@ def test_submit_without_object_and_locale_strings
end
end

def test_submit_with_object_and_nested_lookup
def test_submit_with_object_which_is_overwritten_by_scope_option
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not modified existing tests.

@@ -931,6 +934,21 @@ def test_submit_with_object_and_nested_lookup
end
end

def test_submit_with_object_which_is_namespaced
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add new test about code that I implemented.

@deraru
Copy link
Contributor Author

deraru commented Jan 1, 2017

@rafaelfranca Could you review again?

@deraru
Copy link
Contributor Author

deraru commented Feb 22, 2017

@rafaelfranca Hello. Any updates on this?

@deraru
Copy link
Contributor Author

deraru commented Feb 27, 2018

@rafaelfranca Hello, what should I do for this?

@rafaelfranca
Copy link
Member

Sorry, I dropped the ball here. Can you squash your commits and rebase this branch? The code looks good.

@deraru
Copy link
Contributor Author

deraru commented Feb 27, 2018

Thank you 😄 Got it!

…odule_name/class_name

Currently submit_tag value translation does not support i18n key style
locale key.
It confuses me a bit because many other components support i18n key
style locale key.
I added i18n key style locale key support to submit tag.
@deraru deraru force-pushed the support-i18n-key-in-submit-tag branch from ce9661b to e579c74 Compare February 27, 2018 02:44
@rafaelfranca rafaelfranca merged commit f86b221 into rails:master Feb 27, 2018
@deraru
Copy link
Contributor Author

deraru commented Feb 27, 2018

Wow! I'm so happy because this is a my first time Rails contribution!
Thank you @rafaelfranca !

@deraru deraru deleted the support-i18n-key-in-submit-tag branch February 27, 2018 05:00
@rafaelfranca
Copy link
Member

Thank YOU! And welcome to the list of contributors http://contributors.rubyonrails.org/contributors/rui-onodera/commits

@seppsepp
Copy link

seppsepp commented Mar 2, 2023

@deraru Thanks for your contribution. I'm wondering why this feature does not work as expected for me ...

I got the following namespaced model Attempts::RandomAttempt:

class Attempt < ApplicationRecord
end

class Attempts::RandomAttempt < Attempt
end

In the view helper for the default submit value the code does not reach this line:

https://github.com/rails/rails/blob/main/actionview/lib/action_view/helpers/form_helper.rb#L2682

This line would produce the expected lookup key en.helpers.submit.attempts/random_attempt.create. But one line up i the code the condition object_name.to_s == model.downcase is false because object_name is "attempts_random_attempt" and model is "Random attempt".

That's why the code proceeds with the line defaults << :"helpers.submit.#{object_name}.#{key}" and the lookup key becomes en.helpers.submit.attempts_random_attempt.create with an underscore after attempts which is not expected as far as I understood.

I'm by far not familiar enough with Rails to come up with a better condition in line https://github.com/rails/rails/blob/main/actionview/lib/action_view/helpers/form_helper.rb#L2681 but the current one seems to be too exclusive. Maybe @rafaelfranca can assist?

I'm looking forward to your reply. Thanks a lot in advance.

I'm using Rails 7.0.4.2 btw.

@seppsepp
Copy link

seppsepp commented Mar 2, 2023

For example line https://github.com/rails/rails/blob/main/actionview/lib/action_view/helpers/form_helper.rb#L2674 could be changed from object.model_name.human to object.model_name.param_key which preserves the namespace which otherwise human would remove. I have to admit it feels a bit weird to use a params related method there ¯\_(ツ)_/¯ . I let the ActionView form_helper tests run and they pass with this change.

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.

7 participants