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

🌐 [amp story shopping] i18n "reviews" string capitalization description #38032

Closed

Conversation

processprocess
Copy link
Contributor

@processprocess processprocess commented Apr 7, 2022

Provides clarity in the string description on why the "reviews" string is not capitalized. This was asked during code review and in a comment.

Screen Shot 2022-04-07 at 10 15 05 AM

@amp-owners-bot
Copy link

amp-owners-bot bot commented Apr 7, 2022

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/_locales/en.json

processprocess referenced this pull request Apr 7, 2022
* Sets active product on plp card click.

* basic template switching

* render nested plp in pdp with filtered product

* header typography

* i18n price

* Decouple templates

* pdp cta button styling

* carousel

* carousel corners

* Carousel styling

* Remove unused.

* scrollbar

* product url

* Buy now string

* spacing

* Carousel single.

* carousel scroll to top

* template logic

* Template logic.

* Variable for logic.

* Template logic

* styling

* Reviews link

* buy now button

* Translated review string

* Revert tag click

* space

* reset carousel scroll

* Only card.

* Not selector

* prefix css variables

* Comments

* lint

* Update test

* remove branching logic

* refactor pdpProduct variables.

* refactor isOpen.

* revert

* spelling

* Update example templates

* lint

* update test

* Update visual test

* update visual test to use static images to prevent flakes. img url in plp.

* Update templates for visual tests. Add image to prevent visual diff flakes.

* Open attachment logic

* whitespace

* whitespace

* lint

* Update visual diff test

* visual test
extensions/amp-story/1.0/_locales/en.json Outdated Show resolved Hide resolved
@ilyaspiridonov
Copy link
Contributor

Hey @processprocess thanks for the clarification, but this may actually be an example of a classic problem in localisation :)

Here's why:

  1. In some languages translators may want/need to change the word order (e.g. Arabic)
  2. In some languages reviews will change its form depending on the number that precedes it. Russian is a typical example:
1 отзыв
2 отзыва
86 отзывов

BTW, you'll actually also get 1 reviews in English :)

There are some ways around this that are used in different frameworks (e.g. Pluralisation in Vue i18n), however, the easiest one would be rephrasing the translation - which, again, will often lead to changing the word order:

Отзывов: 86
OR
Отзывы (86)
OR
Отзывы - 86

With that in mind, is there any way we can avoid concatenation - e.g. by using variables?

"string": "$reviews_count$ reviews"

Ideally, concatenation should always be avoided in localization (even when not dealing with variables), as reversed word order is quite common in many languages.

@ilyaspiridonov
Copy link
Contributor

@processprocess should I open an issue for this?

@processprocess
Copy link
Contributor Author

processprocess commented Apr 8, 2022

@processprocess should I open an issue for this?

@ilyaspiridonov that would be great :)
Please assign me to the issue once it's written.

We might need to remove the translated string and use a design pattern like this:
Screen Shot 2022-04-08 at 8 32 04 AM

@ilyaspiridonov
Copy link
Contributor

ilyaspiridonov commented Apr 11, 2022

@processprocess should I open an issue for this?

@ilyaspiridonov that would be great :) Please assign me to the issue once it's written.

Done! #38060
(Not sure I can assign you though... )

We might need to remove the translated string and use a design pattern like this: Screen Shot 2022-04-08 at 8 32 04 AM
This looks good indeed!

What should we do with the 'reviews' string in the meantime from the translation perspective - leave in English for now?
CC @newmuis

@gmajoulet
Copy link
Contributor

Pluralization or string interpolation are out of scope for our localization system.

I think we should either:

1a. Update the description, show how the "review" string will be used, and ask translators to pay attention to whether it should be uppercase or not
1b. Always use plural

or

2a. Remove the "reviews" string and only show the content, as @processprocess suggested

@processprocess
Copy link
Contributor Author

processprocess commented Apr 19, 2022

Closing this PR since it doesn't fix the root of the problem.
As Gabriel mentioned in 2a, just showing the content would be good since it removes this problem.
I updated the description of #38060 to reflect this.

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.

4 participants