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 SlimCut Media ads in amp-ad #7265

Merged
merged 1 commit into from Feb 16, 2017

Conversation

m1ck43l
Copy link
Contributor

@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
Contributor

/to @lannka

@lannka lannka self-assigned this Feb 1, 2017
Copy link
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
Contributor 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
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
Contributor 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
Contributor 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
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
Contributor 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.

None yet

3 participants