Skip to content

Add support for SlimCut Media ads in amp-ad#7265

Merged
lannka merged 1 commit intoampproject:masterfrom
SlimCutMedia:support_scm_ads
Feb 16, 2017
Merged

Add support for SlimCut Media ads in amp-ad#7265
lannka merged 1 commit intoampproject:masterfrom
SlimCutMedia:support_scm_ads

Conversation

@m1ck43l
Copy link
Copy Markdown

@m1ck43l m1ck43l commented Jan 31, 2017

Hello,

I've followed the guidelines and examples to add support for SlimCut Media ads into amp-ad.

Please review this pull request, thanks!

Implements #7254

@jridgewell
Copy link
Copy Markdown
Contributor

/to @lannka

@lannka lannka self-assigned this Feb 1, 2017
Copy link
Copy Markdown
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

The example does not load an ad locally. could you check?

@m1ck43l
Copy link
Copy Markdown
Author

m1ck43l commented Feb 2, 2017

Example was limited to the heroku test site, sorry for that.
The example is now running locally.

We authorize domains localhost and ampscm.herokuapp.com.

Thanks

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Feb 3, 2017

@m1ck43l the example works now. But I see a noticeable blank time between the loading indicator and the ad display. Could you confirm you're calling render-start API at the right time?

@m1ck43l
Copy link
Copy Markdown
Author

m1ck43l commented Feb 6, 2017

@lannka We plan to add a loading screen to avoid the blank time. Do you need it to be implemented right for the PR? Thanks

@m1ck43l
Copy link
Copy Markdown
Author

m1ck43l commented Feb 13, 2017

@lannka We've added the loading screen. The white screen will disappear once we have the ad. Thanks

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Feb 13, 2017

@m1ck43l sorry for the late reply. AMP runtime already provided a loading indicator. Are you able to call render-start at the time you hide your loading screen? If not, I'd prefer no 2nd loading indicator.

@m1ck43l
Copy link
Copy Markdown
Author

m1ck43l commented Feb 15, 2017

@lannka We've removed the second loading indicator. Should be good now.

@lannka lannka merged commit 40e9c32 into ampproject:master Feb 16, 2017
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
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.

3 participants