readthedocs / addons

JavaScript client to integrate with Read the Docs nicely
https://readthedocs-addons.readthedocs.io/
MIT License
22 stars 6 forks source link

EthicalAds: use theme-logic to decide position #309

Closed humitos closed 6 months ago

humitos commented 6 months ago

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

humitos commented 6 months ago

This is how it works now:

Peek 2024-05-16 20-04

It seems there may be something wrong with the classes because it doesn't render properly when it goes to the navbar. Any clue here?

humitos commented 6 months ago

This is how it works in Alabaster-like themes.

Peek 2024-05-16 20-10

humitos commented 6 months ago

Hrm... I was missing including the CSS file 👍🏼 -- it should work after I figure it out how to include it without using web components 😅

humitos commented 6 months ago

With the correct CSS it looks 👍🏼 , but I'm not sure yet why it's not being loaded by default:

Peek 2024-05-16 20-49

humitos commented 6 months ago

It seems it figured it out. I only need to understand why the css that I'm injecting via https://github.com/readthedocs/addons/blob/2e648494da857a139a907d59cf925ee4c5f9ff58/src/ethicalads.js#L4 does not affect the ad. I think @agjohnson may have an idea here.

humitos commented 6 months ago

However, activating the CSS manually looks great!

Peek 2024-05-16 21-22

davidfischer commented 6 months ago

Some of this is historical. When the ad appears in the stickybox in the lower right corner, it uses the CSS directly from the ad client. However, when it appears in the sidebar, it's usually using some custom RTD CSS.

Part of the reason for this is that Alabaster and the RTD look quite different and the exact same ad placement may not work in both. We might want to use the light theme with Alabaster and the dark theme with RTD-like themes. I'm not sure.

humitos commented 6 months ago

I figured this out. I think it's working as expected. Here are some examples:

Theme Image
Read the Docs Screenshot_2024-05-17_11-32-06
Alabaster Screenshot_2024-05-17_11-33-44
Docusaurus Screenshot_2024-05-17_11-34-05
Material for MkDocs Screenshot_2024-05-17_11-34-32
humitos commented 6 months ago

In my opinion, the next step here is to integrate the ad in a better way with other themes/tools. This is, for example, do not use stickybox in Material for MkDocs and place it in the left/right bar.

humitos commented 6 months ago

I added some extra code here to work on Docusaurus and Material for MkDocs in a better integrated way 😄

Theme Image
Docusaurus Screenshot_2024-05-17_12-20-59
Material for MkDocs Screenshot_2024-05-17_12-21-09
humitos commented 6 months ago

IMO, this PR is ready to review and deploy. We can continue iterating and adding more cases in the following PRs.

humitos commented 6 months ago

Added another integration for Sphinx Book Theme (Jupyter Book)

Screenshot_2024-05-17_13-38-00

ericholscher commented 6 months ago

As I mentioned at #295, we might be able to remove some of the isSphinxReadTheDocsLikeTheme()/isSphinxAlabasterLikeTheme() code by using the styling directly from the ad client instead of "readthedocs-sidebar" (just use data-ea-type="image", the default). Using the custom readthedocs-sidebar means you're responsible for the styling basically. I'm not sure this code can be removed 100% because you still might want to inject the ad in different places on different themes but you probably could just accept light/dark mode depending on the theme. However, what you have seems to look good although it does sort of mean RTD continues to maintain custom ad CSS. Perhaps switching to the built-in image placement in a v2?

Yea, if we could get away from custom RTD CSS that would be great. Hopefully the ad styles on the ad client should be enough for most things, but I agree we don't need to block this work on that, so we can merge this and then figure out deprecating the CSS styles.

humitos commented 6 months ago

I'm not opposed to move the CSS styles into the ad client and rely on light/dark mode only. However, we will still need some code in the addons (isReadTheDocsLikeTheme(), etc) to determine where to inject the ad based on the theme/doctool. I'm fine with that since we can give users a better experience than always using the stickybox.

ericholscher commented 6 months ago

@humitos yea, we definitely want logic around where to inject -- I think removing our own custom CSS is the next step 👍