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] Rename tag-id to product-id #37527

Merged
merged 2 commits into from
Jan 31, 2022
Merged

♻️ [amp story shopping] Rename tag-id to product-id #37527

merged 2 commits into from
Jan 31, 2022

Conversation

processprocess
Copy link
Contributor

Makes semantics be more symmetric by using data-product-id instead of data-tag-id.
This helps put emphasis on the product instead of the tag since the product is what's being identified, and not the tag itself.

@processprocess processprocess self-assigned this Jan 31, 2022
@amp-owners-bot
Copy link

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/amp-story-shopping-config.js
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-shopping/0.1/test/test-amp-story-shopping-tag.js
extensions/amp-story/1.0/amp-story-store-service.js

Hey @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-store-service.js

@gmajoulet
Copy link
Contributor

Can you please update the I2I as well?

@processprocess
Copy link
Contributor Author

Can you please update the I2I as well?

Was doing that right now actually :)

@processprocess processprocess mentioned this pull request Jan 31, 2022
Merged
@processprocess processprocess merged commit 365daaa into ampproject:main Jan 31, 2022
@processprocess processprocess deleted the product-id branch January 31, 2022 21:35
samouri pushed a commit to samouri/amphtml that referenced this pull request Feb 2, 2022
@processprocess @samouri
rename key (ampproject#37527)
ba5b6a7
@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

@newmuis newmuis newmuis approved these changes

@jshamble jshamble Awaiting requested review from jshamble

Assignees

@processprocess processprocess

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

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