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 amp-embed type=24smi #12029

Merged
merged 1 commit into from
Nov 22, 2017
Merged

Add amp-embed type=24smi #12029

merged 1 commit into from
Nov 22, 2017

Conversation

TvoroG
Copy link
Contributor

@TvoroG TvoroG commented Nov 13, 2017

Add support for 24smi widgets.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@TvoroG
Copy link
Contributor Author

TvoroG commented Nov 13, 2017

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@ampprojectbot ampprojectbot added this to the Pending Triage milestone Nov 13, 2017
zhouyx
zhouyx suggested changes Nov 17, 2017
View reviewed changes
Copy link
Contributor

@zhouyx zhouyx 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 the Pull Request. Looks good in general. Few requests.

ads/24smi.js
* @param {!Window} global
* @param {!Object} data
*/
export function _24smi(global, data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please name the function 24smi instead. It's much easier for use to maintain code by using same name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be syntax error if I'll remove underscore, because identifiers cannot start by digit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, you're right! I guess we can live with the non-match here : )

ads/_config.js Outdated
@@ -53,6 +53,13 @@ let AdNetworkConfigDef;
* @const {!Object<string, !AdNetworkConfigDef>}}
*/
export const adConfig = {
'24smi': {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 24smi should come after _ping_

ads/_config.js Outdated
@@ -53,6 +53,13 @@ let AdNetworkConfigDef;
* @const {!Object<string, !AdNetworkConfigDef>}}
*/
export const adConfig = {
'24smi': {
prefetch: 'https://jsn.24smi.net/smi.js',
preconnect: [
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to have an array for single preconnect url.

3p/integration.js Outdated
@@ -209,6 +210,7 @@ import {zucks} from '../ads/zucks';
* @const {!Object<string, boolean>}
*/
const AMP_EMBED_ALLOWED = {
'24smi': true,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please fix order

@TvoroG
Copy link
Contributor Author

TvoroG commented Nov 18, 2017

Thanks for the review. I fixed all the problems except function name in ads/24smi.js.

zhouyx
zhouyx suggested changes Nov 22, 2017
View reviewed changes
Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

LGTM with two more requests. Thanks

ads/24smi.js
* @param {!Window} global
* @param {!Object} data
*/
export function _24smi(global, data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, you're right! I guess we can live with the non-match here : )

ads/24smi.md Outdated
For semantics of configuration, please contact [24smi](https://partner.24smi.info).

Required parameters:
- data-src
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: src instead.

3p/integration.js Outdated
@@ -68,6 +68,7 @@ import {twitter} from './twitter';

// 3P Ad Networks - please keep in alphabetic order
import {_ping_} from '../ads/_ping_';
import {_24smi} from '../ads/24smi';
Copy link
Contributor

Choose a reason for hiding this comment

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

one minor request here: Since _24smi should come before _ping in alphabetic order. Can we move the _ping_ (for test purpose) above the comment. Should be

import {_ping_} from '../ads/_ping_';

// comments
import {_24smi}...

@TvoroG
Copy link
Contributor Author

TvoroG commented Nov 22, 2017

Done :)

@zhouyx zhouyx merged commit 36c3439 into ampproject:master Nov 22, 2017
@zhouyx
Copy link
Contributor

zhouyx commented Nov 22, 2017

Looks great! Merged. Thanks for the Pull Request.

ghost pushed a commit that referenced this pull request Dec 6, 2017
@TvoroG
Add amp-embed type=24smi (#12029)
ef00ce0
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@TvoroG @gzgogo
Add amp-embed type=24smi (ampproject#12029)
d3acd05
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

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

Successfully merging this pull request may close these issues.
4 participants
@TvoroG @googlebot @zhouyx @ampprojectbot