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

✨amp-smartlinks: Adding new field to payload for API Endpoint #25160

Merged
merged 2 commits into from Oct 22, 2019
Merged

✨amp-smartlinks: Adding new field to payload for API Endpoint #25160

merged 2 commits into from Oct 22, 2019

Conversation

Asadius
Copy link
Contributor

@Asadius Asadius commented Oct 21, 2019 •
edited

✨ Updated amp-smartlinks extension to pass link_source to Narrativ's Linkmate API endpoint

@amp-owners-bot amp-owners-bot bot requested a review from zhouyx October 21, 2019 17:37
@Asadius Asadius changed the title ♻️ amp-smartlinks: Adding new field to payload for API Endpoint ✨amp-smartlinks: Adding new field to payload for API Endpoint Oct 21, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Oct 21, 2019

cc @PhilWinchester, @pbecotte, @c-nichols who are the owners of the <amp-smartlinks> component.

Question about the newly added field. You mentioned it is used for Narrativ's Linkmate API endpoint, but the field noticed is hardcoded, is it going to be sent along with all requests? Thanks.

@Asadius
Copy link
Contributor Author

Asadius commented Oct 22, 2019

cc @PhilWinchester, @pbecotte, @c-nichols who are the owners of the <amp-smartlinks> component.

Question about the newly added field. You mentioned it is used for Narrativ's Linkmate API endpoint, but the field noticed is hardcoded, is it going to be sent along with all requests? Thanks.

Hi! Thanks for reviewing this PR. To answer your question, we are hardcoding this field here so we send it with all calls from the AMP extension. In the future, if we decide to specify a different link source for amp links for our internal reporting we can do so more easily. cc @PhilWinchester

PhilWinchester
PhilWinchester approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@PhilWinchester PhilWinchester left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

@zhouyx zhouyx merged commit ba95744 into ampproject:master Oct 22, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Oct 22, 2019

Thanks for the clarification. LGTM. PR merged.

joshuarrrr pushed a commit to Parsely/amphtml that referenced this pull request Oct 22, 2019
@Asadius @joshuarrrr
BAM-3538 added field to payload (ampproject#25160)
7f32871
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
@Asadius
BAM-3538 added field to payload (ampproject#25160)
9c58557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouyx zhouyx zhouyx approved these changes

@PhilWinchester PhilWinchester PhilWinchester approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Asadius @zhouyx @PhilWinchester @googlebot