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

Support for responsive adsense banners #56

Merged
merged 10 commits into from
Nov 1, 2016
Merged

Support for responsive adsense banners #56

merged 10 commits into from
Nov 1, 2016

Conversation

gustavofoa
Copy link

@gustavofoa gustavofoa commented Sep 14, 2016

I included support to responsive adsense banners on top/bottom of articles and index. And on bottom of aside.

@@ -25,6 +25,16 @@ The minimalist [Pelican](http://blog.getpelican.com/) theme.
- [Piwik](http://piwik.org/)
- [StatusCake](https://www.statuscake.com/)

## Adsense banner
Copy link
Owner

@alexandrevicenzi alexandrevicenzi Sep 14, 2016

Choose a reason for hiding this comment

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

Settings are documented on Wiki.

@alexandrevicenzi
Copy link
Owner

@gustavofoa nice, but take a look in my comment at #47

@gustavofoa gustavofoa closed this Sep 17, 2016
@gustavofoa gustavofoa reopened this Sep 17, 2016
@alexandrevicenzi
Copy link
Owner

@gustavofoa is this OK?

@alexandrevicenzi alexandrevicenzi added this to the 2.1 milestone Sep 29, 2016
@gustavofoa
Copy link
Author

It is Ok for me.
I put adsense banners where you suggested.
But in aside area I think it didn't fit well, cause the banner causes a vertical scroll bar.

Copy link
Owner

@alexandrevicenzi alexandrevicenzi left a comment

Choose a reason for hiding this comment

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

Looks like if a user uses more than one banner it will load ads JS for each banner, this is a shot in the foot, it will increase page load a lot.

JSs file should be loaded async at the page bottom, but as always google want's to load stuff in the head to keep it slow AF. Not sure if it will work, but adsense docs will say it.

Looks like GitHub is having problems, but I'll change the branch of this PR to another, so it wont break anything.

Also, I'll fix this PR tomorrow.

@@ -77,3 +77,13 @@
ADD_THIS_ID = 'ra-XX3242XX'

USE_LESS = True

ADSENSE = {
'adClientId' : 'ca-pub-XXXXXXXXXXXXXXXX',
Copy link
Owner

Choose a reason for hiding this comment

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

Variables should user only _ not camelCase mixed

@alexandrevicenzi alexandrevicenzi changed the base branch from master to adsense November 1, 2016 04:04
@alexandrevicenzi alexandrevicenzi merged commit 5360d3f into alexandrevicenzi:adsense Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants