-
Notifications
You must be signed in to change notification settings - Fork 20
EthicalAd: fixed footer placement for specific themes #618
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 a POC to show how we can use fixed footer ad on specific known themes. The logic is as follows: - the theme has to be a known theme - if the image ad added to the navbar has to be below the fold - show a fixed footer ad and we add some `padding-bottom` CSS style to the content container (theme-specific selector) with the height of the fixed footer ad (using fixed `47.2px` for now but it can be dynamically calculated) Closes #616
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.
This looks great to me and what I was trying to do in a generic way that didn't work. I think applying it per theme will be a great test to see how it works on the CTR data. I can't speak too much to the CSS, and the right way to actually implement it, but it looks pretty simple.
f3b818d
to
6270ec3
Compare
Read the Docs themeIt works on the content scrolling and also the navigation scrolling. All the content in both is visible when scrolling to the bottom. Peek.2025-07-09.12-31.webm |
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.
This looks good to me with a CSS review from @agjohnson or @davidfischer. I'm excited to test it out!
This is also a place I'd love to see us using stronger accent colors, and doing a bit more to make the ad nice for folks. We should try and make this placement look as nice as the MDN one -- I think some additional CSS customization to compliment the themes could really make it look better, and is worth the investment.
Grab the height of the fixedfooter and use to add some ``padding-bottom`` to the required elements.
if (placementStyle == "fixedfooter") { | ||
// Use a ``MutationObserver`` to listen to style changes in the fixed footer ad. | ||
// Grab the height of it and use to add some ``padding-bottom`` to the required elements. | ||
const config = { attributes: true, childList: false, subtree: false }; |
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.
Is this needed? Did we get this logic from somewhere else? It feels pretty magical to me.
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.
Official MDN docs: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
Cool -- this looks great to me, and I'd be 👍 to ship it. I do think there's some better UX we could be doing, but not sure if we want to block this on that work. |
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.
I'd like to try this with the deploy this week. I think this looks simple enough to QA, so let's just try it on our theme to start, and see how it goes?
Cool! I removed the code for the other themes (material for mkdocs, docusaurus and furo) --we can re-add them later. |
Initial implementation to try out ad in dark mode on ethicalads: - It shows the ad in the defined style at the beginning - It listens to changes in the theme mode and switch ad theme as well - It only works on furo for now - The structure will be the same for other themes, we just need to implement minor details on them ### Sidebar ad Note that I had to use `style=image` instead of `style=readthedocs-sidebar` because the latter doesn't support dark mode. We should probably implement this in the ad client, and switch it back here. [Peek 2025-07-22 11-39.webm](https://github.com/user-attachments/assets/7c4d272c-d13e-44b7-a954-c142b148691e) ### Text ad at the bottom of the page [Peek 2025-07-22 11-40.webm](https://github.com/user-attachments/assets/e01d8718-85c9-44ff-9bf9-641f5e501766) ### Fixed footer text ad Note this is not adding in this PR because it needs this chunk of code to work: [`d7d3c9e` (#618)](d7d3c9e#diff-10e47ee23788d0e8d3d4e13cee726fd04c8fe035922a74ac1c7d432db1dfb9faR102). If we want to have it working, I can bring that code in here. [Peek 2025-07-22 11-46.webm](https://github.com/user-attachments/assets/9630eab2-3da1-469c-aa1a-a80c6e0d85ce) Related readthedocs/meta#192 --------- Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com> Co-authored-by: Eric Holscher <eric@ericholscher.com>
This is a POC to show how we can use fixed footer ad on specific known themes. The logic is as follows:
padding-bottom
CSS style to thecontent container (theme-specific selector) with the height of the fixed
footer ad (using fixed
47.2px
for now but it can be dynamically calculated)The technique of adding the
padding-bottom
to a theme-specific element solves the problem highlighted in the previous approach done in #447Sphinx Furo theme
Peek.2025-07-07.12-59.webm
Material for MkDocs
Peek.2025-07-07.13-10.webm
Closes #616