-
Notifications
You must be signed in to change notification settings - Fork 20
Use footer text content ad for unknown themes #562
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
Conversation
This is much less intrusive than the stickybox, and allows us to still have an ad injected, but not cover the content for users when this happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. My only worry is in the rare case where none of the selectors match and we just append the ad to <body> but that probably won't happen.
Line 400 in 57d34df
| [FALLBACK_DOCTOOL]: ["article", "main", "div.body", "div.document", "body"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good. I haven't tested it locally, tho.
I would prefer to keep using the same ID as we are using for all the other ads, for consistency, tho.
|
Yea, I think we can adjust it a bit as we see how it works, and also of course just not add the ad if we fallback to |
Alternates between using the stickybox placement and the footer placement on unknown themes. It also adds an ID to the stickybox placement so we can see performance of it. As written, this introduces a random element which may not be desirable. We could instead introduce the stickybox only on very wide screens (say >=1600px) or we could base it on the title of the project or something like that to make it deterministic. I'm open to suggestions. > // TODO: Check this placement on the dashboard, > // and see how this is performing. The footer placement is performing very well at ~4x the CTR with ~50% the view rate vs. previously. Ref: #562


This is much less intrusive than the stickybox,
and allows us to still have an ad injected,
but not cover the content for users when this happens.
Replaces #447
Closes #311