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

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

Merged
merged 22 commits into from
Jan 17, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a0a4f47
Stage custom backend request preliminary work
jackbsteinberg Jan 7, 2020
3926f1f
Integrate clientId into data request
jackbsteinberg Jan 7, 2020
5a8d518
Add test and clean up error handling
jackbsteinberg Jan 7, 2020
e2b8343
Polish comments
jackbsteinberg Jan 7, 2020
9a8f3d9
Adjust to be more privacy conscious
jackbsteinberg Jan 7, 2020
58465b6
Update based on design doc comments
jackbsteinberg Jan 10, 2020
d2ea63d
Freeze interaction until data load (while maintaining tap events)
jackbsteinberg Jan 10, 2020
e5e4774
Reconfigure delection delay method
jackbsteinberg Jan 10, 2020
f74115d
Rename for clarity
jackbsteinberg Jan 10, 2020
00beac1
Address wave of review comments
jackbsteinberg Jan 10, 2020
fe5ad26
Add additional edge case handling
jackbsteinberg Jan 13, 2020
6f43a5a
Address review comments
jackbsteinberg Jan 13, 2020
dd78c9c
Add extra checks to cover edge cases and pass tests
jackbsteinberg Jan 14, 2020
6d2eec5
Fix page deeplinking fro reaction id
jackbsteinberg Jan 14, 2020
bce0b65
Clean request execution
jackbsteinberg Jan 14, 2020
41e9e47
Add informative dev errors and refactor for clarity
jackbsteinberg Jan 15, 2020
1e7eabb
Update testing endpoint
jackbsteinberg Jan 15, 2020
8967ff5
Add amp-story-quiz.js to the whitelist for cidForDoc
jackbsteinberg Jan 15, 2020
6fda2c5
Fix incorrect typedef for closure compiler
jackbsteinberg Jan 15, 2020
f2f0aa0
Fix Closure Compiler type issues
jackbsteinberg Jan 16, 2020
2995c72
Adjust bookend config request execution
jackbsteinberg Jan 16, 2020
7301496
Move user selection assignment to synchronous environment
jackbsteinberg Jan 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add additional edge case handling
  • Loading branch information
jackbsteinberg committed Jan 16, 2020
commit fe5ad26539054218320ffc7a4d853c3ddadf2e8b
2 changes: 1 addition & 1 deletion examples/amp-story/quiz.html
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ <h1 id="cat-facts-title">Cat Facts!</h1>
<amp-story-grid-layer>
<amp-story-quiz
id='cat-question-appear'
endpoint="https://google.com">
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

<h1>When did the domestic cat first appear on the scene?</h1>
<option>7,000 years ago</option>
<option correct>500,000 years ago</option>
Expand Down
54 changes: 35 additions & 19 deletions extensions/amp-story/1.0/amp-story-quiz.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {StateProperty, getStoreService} from './amp-story-store-service';
import {closest} from '../../../src/dom';
import {createShadowRootWithStyle} from './utils';
import {dev} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {getRequestService} from './amp-story-request-service';
import {htmlFor} from '../../../src/static-template';
import {toArray} from '../../../src/types';
Expand Down Expand Up @@ -409,27 +410,29 @@ export class AmpStoryQuiz extends AMP.BaseElement {

let URL = this.element.getAttribute('endpoint');

const requestVars = {
reactionType: STORY_REACTION_TYPE_QUIZ,
reactionId: '', // combine url & attribute
};
const requestVars = dict({
'reactionType': STORY_REACTION_TYPE_QUIZ,
'reactionId': `page=${this.storeService_.get(
StateProperty.CURRENT_PAGE_ID
)}&url=${this.win.location.href}`,
});

const requestOptions = {
method: 'GET',
};
const requestOptions = dict({
'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?

requestVars.reactionValue = reactionValue;
requestOptions.method = 'POST';
requestOptions.body = requestVars;
requestVars['reactionValue'] = reactionValue;
requestOptions['method'] = 'POST';
requestOptions['body'] = requestVars;
} else {
URL += `?reactionType=${
requestVars.reactionType
}&reactionId=${encodeURIComponent(requestVars.reactionId)}`;
requestVars['reactionType']
}&reactionId=${encodeURIComponent(requestVars['reactionId'])}`;
}

return this.getClientId_().then(clientId => {
requestVars.clientId = clientId;
requestVars['clientId'] = clientId;
return this.requestService_.executeRequest(URL, null, requestOptions);
});
}
Expand All @@ -454,24 +457,37 @@ export class AmpStoryQuiz extends AMP.BaseElement {
* @private
*/
handleSuccessfulDataRetrieval_(response) {
if (!('data' in response)) {
return;
}

this.responseData_ = {
totalCount: response.data.totalResponseCount,
data: response.data.responses,
};

this.hasUserSelection_ = response.data.hasUserResponded;
if (this.hasUserSelection_) {
this.quizEl_.classList.add('i-amphtml-story-quiz-post-selection');

const selectedOptionKey = this.responseData_.data.find(
response => response.selectedByUser
).reactionValue;

this.quizEl_
.querySelectorAll('.i-amphtml-story-quiz-option')
[selectedOptionKey].classList.add(
'i-amphtml-story-quiz-option-selected'
const options = this.quizEl_.querySelectorAll(
'.i-amphtml-story-quiz-option'
);

if (selectedOptionKey >= options.length) {
dev().error(
TAG,
`Quiz #${this.element.getAttribute('id')} does not have option ${
answerChoiceOptions[selectedOptionKey]
}, but user selected option ${answerChoiceOptions[selectedOptionKey]}`
);
return;
}

this.quizEl_.classList.add('i-amphtml-story-quiz-post-selection');
[selectedOptionKey].classList.add('i-amphtml-story-quiz-option-selected');
}
}
}