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-config data json handle get and set config data #36757

Merged
merged 57 commits into from
Nov 18, 2021

Conversation

jshamble
Copy link
Contributor

@jshamble jshamble commented Nov 4, 2021

Handles loading and setting amp story config data using the store Service.
Updates the tag to display keyed by their product tag ID.

Fixes #36699
Fixes #36911

@amp-owners-bot
Copy link

amp-owners-bot bot commented Nov 4, 2021

Hey @gmajoulet! These files were changed:

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

Hey @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-request-service.js
extensions/amp-story/1.0/amp-story-share.js
extensions/amp-story/1.0/amp-story-store-service.js
extensions/amp-story/1.0/test/test-amp-story-request-service.js

@jshamble jshamble marked this pull request as draft November 4, 2021 03:36
@jshamble jshamble requested review from coreymasanto and processprocess and removed request for mszylkowski November 4, 2021 04:37
@ampproject ampproject deleted a comment from lgtm-com bot Nov 4, 2021
Copy link
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.

Looking good. Lots of little comments but this is a big PR with lots of good work!

examples/amp-story/amp-story-shopping-inline.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping-remote.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping-remote.html Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story-store-service.js Outdated Show resolved Hide resolved
Copy link
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.

Awesome progress! Some nits and comments.

examples/amp-story/amp-story-shopping.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping.html Show resolved Hide resolved
Copy link
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.

A few more nits and changes. Looking awesome!

Please be sure to update the visual diff test with JSON data too!
examples/visual-tests/amp-story/amp-story-shopping.html

examples/amp-story/amp-story-shopping.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping.html Outdated Show resolved Hide resolved
examples/amp-story/amp-story-shopping.html Outdated Show resolved Hide resolved
@ampproject ampproject deleted a comment from processprocess Nov 5, 2021
@ampproject ampproject deleted a comment from lgtm-com bot Nov 13, 2021
Copy link
Contributor

@coreymasanto coreymasanto left a comment

Choose a reason for hiding this comment

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

I have a few last comments, but this PR otherwise looks good to me!

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

last 2 nits! Looking awesome!

extensions/amp-story/1.0/amp-story-share.js Outdated Show resolved Hide resolved
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.

[amp story shopping] Add Object typedef for shopping data [amp story shopping] Get and set JSON data
5 participants