EthicalAd: use placement-bottom to show the ad above the flyout
#295
Conversation
Instead of injecting the EthicalAd in a customized way, we will injecting it by using the addons approach. This will be a starting test before enabling this in all the projects that are using addons. Requires: * readthedocs/addons#295
src/ethicalads.js
Outdated
| placement.classList.add("raised"); | ||
|
|
||
| // TODO: find the right last resourse to append (probably not `document.body`) | ||
| let main = document.querySelector("[role=main]") || document.body; |
There was a problem hiding this comment.
I think we've written this "Find main div" logic in a few places -- we should standardize on it. I think search is probably the most built out.
Also should we bring over any of the logic around theme placements (https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/core/static-src/core/js/doc-embed/rtd-data.js#L10-L18)? It's pretty Sphinx specific, but probably still useful to have the ability to smartly place things.
Looking at the old code though, it looks like we're only injecting the ad if we're confident that we know what theme it is, so this is probably a better place to start, and not try to make it too smart and see how it works :)
@davidfischer curious about your thoughts here.
There was a problem hiding this comment.
Thinking something like:
Placement preference for RTD ad injection:
Using stickybox image (These float if the screen is wide, automatically):
* Sidebar like content (`role=sidebar`)
(Currently we check for sidebar height, and if it's too long, we put it in the footer -- but this is probably not necessary?)
* Footer like content (`role=footer`)
* Main body content (`role=main`)
Using text:
* Fixed footer (Not conflicting with the flyout?)
Though we should also support the HTML elements (<main>) and perhaps use standardized ARIA roles? 🤷 https://www.w3.org/TR/wai-aria/#role_definitions
There was a problem hiding this comment.
I opened #309 with the logic from the old implementation.
There was a problem hiding this comment.
As to finding the main div, this is what the ad analyzer does. It follows these in order.
As Eric mentioned, we styled the ad based on the theme if we were confident in the theme being RTD-like or Alabaster-like (including RTD-like on mkdocs). This is mostly because Alabaster is "light" while RTD theme is "dark". The current ad client has light and dark theme and that could be used instead if desired. It might allow us to remove some ads-specific code in the RTD codebase. The end goal is just to have the ad look good on a variety of themes regardless of how that actually happens.
The logic/idea is borrowed from the old implementation at https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/core/static-src/core/js/doc-embed/sponsorship.js#L51 Related https://github.com/readthedocs/addons/pull/295/files#r1600336686 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: humitos <humitos@users.noreply.github.com> Co-authored-by: Žan Anderle <zan.anderle@gmail.com>
…stickybox-placement
Instead of injecting the EthicalAd in a customized way, we will injecting it by using the addons approach. This will be a starting test before enabling this in all the projects that are using addons. Requires: * readthedocs/addons#295
This PR enables showing EthicalAds for all projects hosted on Read the Docs and using the new beta addons.
Requires readthedocs/ethical-ad-client#188