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 exco as new type for amp-ad #34613

Merged
merged 2 commits into from
Aug 9, 2021
Merged

✨ Add exco as new type for amp-ad #34613

merged 2 commits into from
Aug 9, 2021

Conversation

dleleko-exco
Copy link
Contributor

Add new type for amp-ad extension:

<amp-ad
  type="exco"
  data-id="3cc23005-ddb5-42c7-965d-37ccf310b1a5"
>
</amp-ad>

@CLAassistant
Copy link

CLAassistant commented May 28, 2021

CLA assistant check
All committers have signed the CLA.

@dleleko-exco
Copy link
Contributor Author

@kristoferbaxter hello! Is there a possibility you can review this changes in near future?

@calebcordry calebcordry self-requested a review August 6, 2021 20:31
@calebcordry
Copy link
Member

@dleleko-exco sorry for the delay, could you please rebase again since this has been pending for a while? I think our lint rules have changed slightly.

Copy link
Member

@calebcordry calebcordry left a comment

Choose a reason for hiding this comment

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

LGTM other than the import changes.

*/

// src/polyfills.js must be the first import.
import '../polyfills';
Copy link
Member

Choose a reason for hiding this comment

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

we changed these import paths while this was pending, sorry. The new structure should be something like

import '#3p/polyfills';

import {draw3p, init} from '#3p/integration-lib';
import {register} from '#3p/3p';

import {exco} from '#ads/vendors/exco';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* limitations under the License.
*/

import {validateData, writeScript} from '../../3p/3p';
Copy link
Member

Choose a reason for hiding this comment

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

from '#3p/3p'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@calebcordry calebcordry 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 contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants