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

✨Add support for AMP Story Quiz Reaction API calls #26242

Merged
merged 22 commits into from
Jan 17, 2020
Merged

✨Add support for AMP Story Quiz Reaction API calls #26242

merged 22 commits into from
Jan 17, 2020

Conversation

jackbsteinberg
Copy link
Contributor

@jackbsteinberg jackbsteinberg commented Jan 7, 2020 •
edited
Loading

Adds support for API calls in AMP Story Quizzes, to both retrieve and update the Reactions data. Currently the API calls must be supplied an endpoint, support for a default endpoint will come in a future PR.

Adds work related to tracking issue #25615

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jan 7, 2020

Hey @gmajoulet, these files were changed:

  • extensions/amp-story/1.0/amp-story-quiz.js
  • extensions/amp-story/1.0/amp-story-request-service.js
  • extensions/amp-story/1.0/test/test-amp-story-quiz.js

Hey @newmuis, these files were changed:

  • extensions/amp-story/1.0/amp-story-quiz.js
  • extensions/amp-story/1.0/amp-story-request-service.js
  • extensions/amp-story/1.0/test/test-amp-story-quiz.js

@newmuis
Copy link
Contributor

newmuis commented Jan 7, 2020

Would be great to pull the API out into a separate doc or GH issue for review

@jackbsteinberg
Copy link
Contributor Author

Would be great to pull the API out into a separate doc or GH issue for review

I've got a doc I'm working on for review (hence why this is a draft PR still) which I should have ready to send out tomorrow

@jackbsteinberg jackbsteinberg marked this pull request as ready for review January 10, 2020 20:26
@amp-owners-bot amp-owners-bot bot requested a review from Enriqe January 10, 2020 20:26
Enriqe
Enriqe reviewed Jan 10, 2020
View reviewed changes
newmuis
newmuis reviewed Jan 10, 2020
View reviewed changes
Copy link
Contributor

@newmuis newmuis left a comment

Choose a reason for hiding this comment

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

Is the plan to submit this with the work-in-progress API and subsequently iterate, or are you hoping to solidify the API design first before merging this PR?

gmajoulet
gmajoulet reviewed Jan 13, 2020
View reviewed changes
examples/amp-story/quiz.html Outdated
@@ -64,7 +64,8 @@ <h1 id="cat-facts-title">Cat Facts!</h1>
</amp-story-grid-layer>
<amp-story-grid-layer>
<amp-story-quiz
id='cat-question-appear'>
id='cat-question-appear'
endpoint="http://localhost:8000">
Copy link
Contributor

Choose a reason for hiding this comment

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

We can revisit later but I'd really like us to consider both approaches to providing configuration to a quiz before we launch, parameters vs JSON

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can sync about this option a bit more tomorrow offline

extensions/amp-story/1.0/amp-story-quiz.js Outdated
'method': 'GET',
});

if (reactionValue !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you deciding whether we're fetching the data or answering the quiz with this test?
If yes, I think we need a clearer way to do so. Maybe two different methods that re-use some factorized code? Should not be a big change but I think we'd gain in clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I have it now is that I have two methods, retrieveReactionData_ and updateReactionData_, which both call executeReactionRequest_.

retrieveReactionData_ does so without passing parameters, where updateReactionData_ passes a reactionValue parameter. I then use the presence of reactionValue to check which reaction data method was called. Do I need to split the two further / bring more calculation into the caller functions?

@jackbsteinberg
Copy link
Contributor Author

PTAL

gmajoulet
gmajoulet reviewed Jan 15, 2020
View reviewed changes
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.

LGTM, just a few nits but it's looking good :))

@jackbsteinberg jackbsteinberg requested a review from zhouyx January 15, 2020 19:54
@jackbsteinberg
Copy link
Contributor Author

Hey @zhouyx, requesting your review for the usage of cidForDoc here. We've been through privacy review on using clientId in this context, ping me for more details if you need!

rsimha
rsimha approved these changes Jan 15, 2020
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Change to build-system/tasks/presubmit-checks.js LGTM.

I'll let someone from the STAMP team comment on whether it's actually okay to use cidForDoc.

gmajoulet
gmajoulet approved these changes Jan 16, 2020
View reviewed changes
@gmajoulet gmajoulet self-requested a review January 17, 2020 16:56
danielrozenberg
danielrozenberg approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

Approving to override bundle-size bot bug

@jackbsteinberg jackbsteinberg merged commit ce6cd7a into ampproject:master Jan 17, 2020
@jackbsteinberg jackbsteinberg deleted the reaction-endpoint branch January 17, 2020 17:08
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Jan 20, 2020
@robinvanopstal
Merge branch 'master' into component-intro-tables
fd92c68 
* master:
  [yahoonativeads-amp] code cleanup and bug fix (ampproject#26325)
  rephrased reasoning for text node  (ampproject#26393)
  Render video alt and title attributes in vertical rendering mdoe. (ampproject#26370)
  Revert "Update I2I & I2S to reflect new Open Source process (ampproject#25530)" (ampproject#26392)
  Skip amp story affiliate link test (ampproject#26386)
  Update I2I & I2S to reflect new Open Source process (ampproject#25530)
  ✨ Add support for `"intrisic"` layout for `<amp-script>` (ampproject#26369)
  📖 Rename Dev Channel to Experimental Channel in docs and comments (ampproject#26255)
  ✨Add support for AMP Story Quiz Reaction API calls (ampproject#26242)
  clarified text node behavior (ampproject#26376)

# Conflicts:
#	extensions/amp-script/amp-script.md
#	extensions/amp-timeago/amp-timeago.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouyx zhouyx zhouyx left review comments

@gmajoulet gmajoulet gmajoulet approved these changes

@danielrozenberg danielrozenberg danielrozenberg approved these changes

@rsimha rsimha rsimha approved these changes

@newmuis newmuis Awaiting requested review from newmuis

@Enriqe Enriqe Awaiting requested review from Enriqe

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jackbsteinberg @newmuis @gmajoulet @danielrozenberg @zhouyx @Enriqe @rsimha @googlebot @ampprojectbot