✨Add support for AMP Story Quiz Reaction API calls #26242
Conversation
|
Hey @gmajoulet, these files were changed:
Hey @newmuis, these files were changed:
|
|
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 |
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"> | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We can sync about this option a bit more tomorrow offline
| 'method': 'GET', | ||
| }); | ||
|
|
||
| if (reactionValue !== undefined) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
PTAL |
|
Hey @zhouyx, requesting your review for the usage of |
jackbsteinberg
force-pushed
the
reaction-endpoint
branch
from
1a73e28
to
7301496
Compare
* 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
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