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

Conversation

Projects
None yet
4 participants
@davidfischer
Contributor

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

@davidfischer davidfischer requested a review from ericholscher Aug 8, 2018

@ericholscher

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/

@humitos

humitos approved these changes Aug 9, 2018

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

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 9, 2018

Contributor

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.

Contributor

davidfischer commented Aug 9, 2018

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

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Aug 9, 2018

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. 👍

Member

ericholscher commented Aug 9, 2018

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidfischer davidfischer deleted the davidfischer/ads-on-pallets-themes branch Aug 10, 2018

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 10, 2018

Member

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".

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

This comment has been minimized.

Show comment
Hide comment
@davidism

davidism 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. 😉

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