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

Do not export empty translation on Android #72

Merged
merged 3 commits into from
Mar 2, 2022

Conversation

ferrandp
Copy link
Contributor

@ferrandp ferrandp commented Mar 1, 2022

Android can handle when a translation is missing, but if the translation is empty Android will display an empty string, which is not what we want.

@ferrandp ferrandp requested a review from jvigne March 1, 2022 14:38
Copy link
Contributor

@jvigne jvigne left a comment

Choose a reason for hiding this comment

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

With a method has_value? in the Translation entity and another one has_translations? in TranslationGroupViewModel you can refactor the code like this :

# translation.rb
def has_value?
  value.present?
end

# translation_group_view_model.rb
def has_translations?
  translation_view_models.present?
end

# strings_serializer.rb
def map_singulars(translations:)
  translations.select(&:has_value?).map { |translation| @translation_mapper.map(translation: translation) }
end

def map_plurals(plurals:)
  plurals
    .select(&:has_value?)
    .map { |label, translations| @translation_group_mapper.map(label: label, translations: translations) }
    .select(&:has_translations?)
end

Otherwise you can also write it like this

def map_singulars(translations:)
  translations
    .select { |translation| translation.value.present? }
    .map { |translation| @translation_mapper.map(translation: translation) }
end

def map_plurals(plurals:)
  plurals
    .select { |translation| translation.value.present? }
    .map { |label, translations| @translation_group_mapper.map(label: label, translations: translations) }
    .select { |translation| translation.translation_view_models.present? }
end

@ferrandp ferrandp requested a review from jvigne March 1, 2022 15:40
@@ -10,6 +10,10 @@ def initialize(label:, translation_view_models:)
@label = label
@translation_view_models = translation_view_models
end

def has_translations?
(translation_view_models || []).any? { |translation| translation.value.present? }
Copy link
Contributor

@jvigne jvigne Mar 2, 2022

Choose a reason for hiding this comment

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

Just one more change : .any? { |translation| translation.value.present? } => .any?(&:has_value?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are translation ViewModels, not translations so they have no has_value method. I did not know if it makes sense to add such a method for a ViewModel. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, you are right. In that case we can add an has_value (boolean) attribute in TranslationViewModel here and set it in the mapper here using the has_value? method. We would have any?(&:has_value). What do you think of this option ?

Copy link
Contributor

@jvigne jvigne left a comment

Choose a reason for hiding this comment

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

One more change and we are good

Copy link
Contributor

@jvigne jvigne left a comment

Choose a reason for hiding this comment

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

Thank you for this PR

@ferrandp ferrandp merged commit 76b6209 into main Mar 2, 2022
@ferrandp ferrandp deleted the feature/skip_empty_strings_android branch March 2, 2022 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants