-
Notifications
You must be signed in to change notification settings - Fork 0
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 support for strings in gems #45
Conversation
f139220
to
bfa8ff9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Quite a large PR but I am very happy with the refactoring that has been done and the tests that have been written.
@@ -1,3 +1,8 @@ | |||
## Unreleased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea to include unreleased changes in the CHANGELOG 👍🏻
else | ||
redirect_back_or_to moirai_translation_files_path, status: :unprocessable_entity | ||
end | ||
redirect_back_or_to moirai_translation_files_path, status: :unprocessable_entity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this simplification
locale ||= I18n.locale | ||
moirai_translations[locale.to_sym][key] | ||
moirai_translations[locale.to_sym].select do |_filename, data| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not filter_map
?
moirai_translations[locale.to_sym].select do |_filename, data| | |
moirai_translations[locale.to_sym].filter_map { |filename, data| filename if data.dig(*key.split(".")).present? } | |
.sort_by { |file_path| file_path.start_with?(Rails.root.to_s) ? 0 : 1 } |
@@ -15,12 +15,25 @@ def call | |||
changes | |||
end | |||
|
|||
def best_file_path_for(key, locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
|
||
visit "/?locale=de&moirai=true" | ||
refute_key_editable page, "buttons.very.much.nested.only_english" | ||
refute_key_editable page, "buttons.very.much.nested.only_italian" | ||
assert_key_editable page, "buttons.very.much.nested.only_english" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice assertion helpers!
@@ -3,6 +3,7 @@ | |||
Moirai::Engine.routes.draw do | |||
root to: "translation_files#index" | |||
|
|||
resources :translations, only: %i[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the addition of the translations controller, but I lack a link to it for example from the /moirai
page. Would this be something to take into consideration or should we just keep it for debugging purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment is very ugly. We could for sure link it, but is also not documented at the moment, so let's keep it for debugging purpose for now.
Co-authored-by: Daniel Bengl <53896675+CuddlyBunion341@users.noreply.github.com>
This PR adds support for both strings coming from gems and strings not yet translated in the current language.
It enables to widen much more the range of strings that is possible to translate with moirai.
Replaces #32