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.

Sign up for GitHub

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

Jump to bottom

♻️ ✅ [amp story shopping] Move i18n CTA label logic into shopping-attachment component #37515

Merged
merged 1 commit into from
Jan 31, 2022
Merged

♻️ ✅ [amp story shopping] Move i18n CTA label logic into shopping-attachment component #37515

merged 1 commit into from
Jan 31, 2022

Conversation

processprocess
Copy link
Contributor

This PR encapsulates shopping attachment logic by setting the cta-text attribute on the inner page-attachment element and includes a unit test.

@processprocess processprocess self-assigned this Jan 28, 2022
jshamble
jshamble approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@jshamble jshamble left a comment

Choose a reason for hiding this comment

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

LGTM!

nit: It may be worth playing around and refactoring some of the calls at the beginning of some of the in the unit tests into a helper function to enforce DRY.

@amp-owners-bot
Copy link

Hey @gmajoulet! These files were changed:

extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js
extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-attachment.js

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

extensions/amp-story-page-attachment/0.1/amp-story-open-page-attachment.js

@processprocess processprocess merged commit 0eab595 into ampproject:main Jan 31, 2022
samouri pushed a commit to samouri/amphtml that referenced this pull request Feb 2, 2022
@processprocess @samouri
Encapsulate shopping label functionality. (ampproject#37515)
bf2011b
@processprocess processprocess mentioned this pull request Feb 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@jshamble jshamble jshamble approved these changes

@mszylkowski mszylkowski Awaiting requested review from mszylkowski

Assignees

@processprocess processprocess

Labels
P1: High Priority PR use: In Beta / Experimental PR use: In LTS PR use: In Stable WG: stories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@processprocess @gmajoulet @jshamble @ampprojectbot