Skip to content

✨ amp-story-shopping-config data json handle get and set config data#36757

Merged
jshamble merged 57 commits intoampproject:mainfrom
jshamble:shopDataJsonHandle
Nov 18, 2021
Merged

✨ amp-story-shopping-config data json handle get and set config data#36757
jshamble merged 57 commits intoampproject:mainfrom
jshamble:shopDataJsonHandle

Conversation

@jshamble
Copy link
Copy Markdown
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 amp-owners-bot Bot requested a review from mszylkowski November 4, 2021 03:36
@amp-owners-bot
Copy link
Copy Markdown

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
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.

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

Comment thread examples/amp-story/amp-story-shopping-inline.html Outdated
Comment thread examples/amp-story/amp-story-shopping-remote.html Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js Outdated
Comment thread examples/amp-story/amp-story-shopping-remote.html Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js Outdated
Comment thread extensions/amp-story/1.0/amp-story-store-service.js Outdated
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.

Awesome progress! Some nits and comments.

Comment thread examples/amp-story/amp-story-shopping.html
Comment thread examples/amp-story/amp-story-shopping.html Outdated
Comment thread examples/amp-story/amp-story-shopping.html
Comment thread examples/amp-story/amp-story-shopping.html
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
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.

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

Comment thread examples/amp-story/amp-story-shopping.html Outdated
Comment thread examples/amp-story/amp-story-shopping.html Outdated
Comment thread examples/amp-story/amp-story-shopping.html Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js Outdated
Comment thread examples/amp-story/amp-story-shopping.html Outdated
@ampproject ampproject deleted a comment from processprocess Nov 5, 2021
@ampproject ampproject deleted a comment from lgtm-com Bot Nov 13, 2021
@jshamble jshamble requested a review from gmajoulet November 15, 2021 19:28
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.css
Comment thread extensions/amp-story/1.0/amp-story-store-service.js Outdated
Comment thread extensions/amp-story/1.0/amp-story-store-service.js Outdated
Comment thread extensions/amp-story/1.0/amp-story-store-service.js Outdated
Comment thread extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-tag.js Outdated
Comment thread extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-tag.js Outdated
Comment thread extensions/amp-story/1.0/test/test-amp-story-request-service.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Copy link
Copy Markdown
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!

Comment thread examples/visual-tests/amp-story/amp-story-shopping.html Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js Outdated
Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-tag.js
Comment thread extensions/amp-story/1.0/test/test-amp-story-request-service.js
Copy link
Copy Markdown
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!

Comment thread extensions/amp-story-shopping/0.1/amp-story-shopping-config.js Outdated
Comment thread extensions/amp-story/1.0/amp-story-share.js Outdated
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