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

Support ads on pallets themes #4499

Merged
merged 2 commits into from
Aug 10, 2018
Merged

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Aug 8, 2018

This will allow showing ads on "alabaster-like" themes which includes the pallets themes. It also no longer handles celery as a special case but as another "alabaster-like" theme.

Testing

To test on pallets themes you can:

Screenshots

A sidebar ad

screen shot 2018-08-08 at 4 29 10 pm

A footer ad

screen shot 2018-08-08 at 4 29 35 pm

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this expands the scope of our ads to a lot more themes. This isn't inherently bad, but we should know that this is the outcome that we're wanting. I imagine it should be fine, but probably worth testing on some of the other common themes.

For example, I believe it will inject on the pypa theme as well: http://pip.readthedocs.io/en/stable/

return constants.THEME_RTD;
} else if ($('div.sphinxsidebar > div.sphinxsidebarwrapper').length === 1) {
return constants.THEME_ALABASTER;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this unique to alabaster? It feels like something that would be on almost all themes, as it's inherited from the Sphinx basic theme: https://github.com/sphinx-doc/sphinx/blob/master/sphinx/themes/basic/layout.html#L52-L53

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Maybe I need to rethink this. That's a good point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using the same selector to inject the Ads in this line https://github.com/rtfd/readthedocs.org/pull/4499/files#diff-549a0ee75d29b10f27785c6d36e8ce2aR28

So, if the "semantic meaning" of that element from the base theme is what we are selecting, it may makes sense on other themes also.

It would be good to test other themes, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it's a common element across most themes with sidebars. Doing that hueristic selector would definitely let us put ads on more themes, but we should just know that's what we're doing :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My immediate goal is to get ads on the pallets themes (which all have different names). Maybe I'll add some constants for "alabaster-like" theme names. From there if we want to iterate and put ads on more themes we can do that in the future.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes make sense to me.

It seems that only the heuristic to detect the theme was changes. The heuristic still looks fragile, but we got here with a similar one, so I'd say that it's OK.

@davidfischer
Copy link
Contributor Author

I reworked this PR a bit to explicitly enumerate the themes rather than using heuristics to detect whether ads would be supported. We might go down that route later, but I don't want to add unintended consequences.

@ericholscher
Copy link
Member

Looks good. As a note, I'm +1 in general on using the sidebar heuristic at some point to enable ads on more themes. I think it's going to be an easy way to get most of the common themes, and a lot of the long tail. It's just something we need to do when we have time to message and support it. 👍

@davidfischer davidfischer merged commit ea7d37e into master Aug 10, 2018
@davidfischer davidfischer deleted the davidfischer/ads-on-pallets-themes branch August 10, 2018 19:35
@humitos
Copy link
Member

humitos commented Aug 10, 2018

I liked the latest changes. Although, it doesn't change the heuristic to decide if it's an ALABASTER-like theme or not. We are still using the same check (> selector on sidebar divs which is from base theme).

I'm 👍 with that, but I understood that you wanted to avoid it for now because it was "too general".

@davidism
Copy link

davidism commented Aug 11, 2018

I don't particularly care what it's called internally, but technically Alabaster is Flask-like, not the other way around. And Flask is derived from Basic, so really they're all Basic-like. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants