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

Adaptive ad styling, based on prefers-color-scheme #37

Closed
pradyunsg opened this issue Nov 2, 2020 · 5 comments · Fixed by #44
Closed

Adaptive ad styling, based on prefers-color-scheme #37

pradyunsg opened this issue Nov 2, 2020 · 5 comments · Fixed by #44
Labels
Accepted Accepted issue on our roadmap

Comments

@pradyunsg
Copy link

Context

I'm trying to figure out how to work with ReadTheDocs' integration with Ethical Ads, in Furo.

Based on https://github.com/readthedocs/readthedocs.org/blob/5ff3c09661460da661171c17d0092086419d8e37/readthedocs/core/static-src/core/js/doc-embed/sponsorship.js#L35, it seems like I can directly add a div, with the correct data-ea-publisher like:

<div
    id="furo-sidebar-ad-placement"
    class="flat"
    data-ea-publisher="readthedocs"
    data-ea-type="readthedocs-sidebar"
    data-ea-manual="true">
</div>

Since Furo is written in a manner that respects prefers-color-scheme media query, this results in Ethical Ads being very bright in dark mode.

Screenshot 2020-11-02 at 3 14 19 PM

Feature Request

Ideally, there'd be a class to say "use light or dark based on prefers-color-scheme", in addition to the existing dark and light classes. I propose adaptive as the name.

The way I'm imagining it'd work is that:

  • For adaptive set on the data-ea-publisher element, the styling would be based on prefers-color-scheme (dark when prefers-color-scheme: dark, light otherwise). This would be done purely in the SCSS.
  • If the class is changed to dark or light (via JS -- toward Customization to select light/dark/dual mode pradyunsg/furo#24), it'd respect that. Assuming we do it all in CSS, that shouldn't be too difficult. :)

Let me know if this sounds like a good idea. :)

@davidfischer
Copy link
Contributor

Firstly, thanks for your interest here!

Ideally, there'd be a class to say "use light or dark based on prefers-color-scheme", in addition to the existing dark and light classes. I propose adaptive as the name.

I think this is a great idea. I like it a lot actually. I wasn't familiar with prefers-color-scheme but it seems like a fit here. By default we'd just use the light scheme but we could use the adaptive one if requested.

it seems like I can directly add a div, with the correct data-ea-publisher like

You are definitely getting ahead of us 😄 . I am actively working on detailing how this will work. We have a working document but it isn't quite ready for public consumption. This will probably change over the next couple weeks or so (see readthedocs/readthedocs.org#7623 (comment)) but I'm in the process of firming it up now.

@pradyunsg
Copy link
Author

pradyunsg commented Nov 3, 2020

You are definitely getting ahead of us

I'd call it following closely. :P

TBH, the motivation here is that the RTD embed logic detects Furo to be alabaster-like and adds the vertical padding above the ad, which makes stuff look bad. I ended up coming back to take a look and then saw "oh, hey, this got an update recently!", which eventually led to this. 🙃

I'm in the process of firming it up now.

Sounds like I've put this on your radar now, if it wasn't already, so yay?

Let me know if there's anything I can help with, for this as well as other stuff where my skillset might be handy. :)

@pradyunsg
Copy link
Author

FYI -- on Furo's end, I've stylized the ad's contents to fit-in with the rest of the sidebar, to based on what is currently being served on pip's documentation. If you reckon that it's NOT OK to depend on the structure of the currently served ads, please holler!

pradyunsg/furo@3285f3b

@pradyunsg
Copy link
Author

pradyunsg commented Nov 14, 2020

PS: pip's (latest) documentation now uses Furo and serves Ethical ads as well! IDK if the ~500k views that pip's documentation gets would matter for Ethical Ads, but hey, pretty it sure won't hurt. :)

@ericholscher
Copy link
Member

@pradyunsg It definitely helps -- we appreciate the work you're doing here, and hopefully we can get the CSS fixes in for this here soon.

@ericholscher ericholscher added the Accepted Accepted issue on our roadmap label Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants