Skip to content

🌐 Internationalization for CTA Text for Amp Story Shopping Component#37008

Merged
jshamble merged 30 commits intoampproject:mainfrom
jshamble:setctatxt
Dec 2, 2021
Merged

🌐 Internationalization for CTA Text for Amp Story Shopping Component#37008
jshamble merged 30 commits intoampproject:mainfrom
jshamble:setctatxt

Conversation

@jshamble
Copy link
Copy Markdown
Contributor

@jshamble jshamble commented Nov 19, 2021

Fixes #36698

@jshamble jshamble mentioned this pull request Nov 19, 2021
Closed
processprocess
processprocess requested changes Nov 19, 2021
View reviewed changes
processprocess
processprocess requested changes Nov 19, 2021
View reviewed changes
processprocess
processprocess reviewed Nov 22, 2021
View reviewed changes
processprocess
processprocess reviewed Nov 23, 2021
View reviewed changes
@jshamble jshamble marked this pull request as ready for review November 23, 2021 18:26
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Nov 23, 2021 •
edited
Loading

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js
extensions/amp-story/1.0/_locales/en.json
extensions/amp-story/1.0/amp-story-open-page-attachment.js
extensions/amp-story/1.0/amp-story-store-service.js
src/service/localization/strings.js

Hey @newmuis! These files were changed:

extensions/amp-story/1.0/_locales/en.json
extensions/amp-story/1.0/amp-story-open-page-attachment.js
extensions/amp-story/1.0/amp-story-store-service.js
src/service/localization/strings.js

processprocess
processprocess reviewed Nov 29, 2021
View reviewed changes
@processprocess
Copy link
Copy Markdown
Contributor

processprocess commented Nov 30, 2021

As per the conversation with @newmuis, the i18n service does not currently support placeholders.
This will allow the code to be simplified, we no longer need get a product name and we can use one translation key.

Please update this PR to:

  1. Use one i18n string: "Shop Now".
  2. Use "Shop Now" in both one product and multiple product cases.

This logic can be applied here.

processprocess
processprocess reviewed Nov 30, 2021
View reviewed changes
processprocess
processprocess approved these changes Dec 1, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

Approving with one nit, pending @coreymasanto's review.

coreymasanto
coreymasanto approved these changes Dec 1, 2021
View reviewed changes
@jshamble jshamble enabled auto-merge (squash) December 2, 2021 06:26
@jshamble jshamble disabled auto-merge December 2, 2021 06:28
@jshamble jshamble enabled auto-merge (squash) December 2, 2021 06:29
@jshamble jshamble merged commit 4ecf84d into ampproject:main Dec 2, 2021
@processprocess
Copy link
Copy Markdown
Contributor

processprocess commented Jan 27, 2022

Looking back at this PR also. We have a visual test but no unit test.
We should do our best to write both unit and visual tests in case one of the tests are down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coreymasanto coreymasanto coreymasanto approved these changes

@processprocess processprocess processprocess approved these changes

Assignees

No one assigned

Labels

PR use: In Beta / Experimental PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[amp story shopping] Set CTA text

4 participants

@jshamble @processprocess @coreymasanto @ampprojectbot