EthicalAds: Default to fixedfooter for smaller screens

[ericholscher]

This is something I've wanted for a little while.
I don't think we're quite ready to ship it,
but I think it makes sense in a lot of cases.

I'm generally becoming more excited about this placement,
especially when the floating one is pretty intrusive.

[ericholscher]

Marking this ready for review after our previous conversation. I'd like to see us doing the fixed footer by default more, since I think it's a much more user friendly ad format. We're hopefully gonna work on a nice fixed footer/header format with a small image, which I think will be even better, but I think this is a nicer UX for default.

In general I really dislike how not integrated the ad feels on non-Sphinx themes, so I think this is a "gentler" approach to ads that isn't quite as impactful for users who want to move to RTD for non-Sphinx themes.

NOTE: This is somewhere that it would be interesting to have a feature flag, so that we could test it a bit before rolling it out more widely.

[ericholscher]

Looks like this hides the "sidebar" toggle on docify: https://test-builds.readthedocs.io/en/docsify/#/ -- and there wasn't an element I could find to add margin-bottom to, because I think they're fixed positioning....

The struggle of placement on arbitrary content :(

[humitos]

In general I really dislike how not integrated the ad feels on non-Sphinx themes, so I think this is a "gentler" approach to ads that isn't quite as impactful for users who want to move to RTD for non-Sphinx themes.

Yeah, I think we should keep adding more frameworks to our heuristic detection and create better ad placement for specific documentation tools. Sphinx is well integrated because we did the work there. The same happens with all the other we have listed at:

addons/src/ethicalads.js

Lines 73 to 142 in 8ccaa33

	if (docTool.isSphinxReadTheDocsLikeTheme()) {
	selector = "nav.wy-nav-side > div.wy-side-scroll";
	element = document.querySelector(selector);
	if (this.elementAboveTheFold(element)) {
	placement.setAttribute("data-ea-type", "readthedocs-sidebar");
	placement.classList.add("ethical-rtd");
	placement.classList.add("ethical-dark-theme");
	knownPlacementFound = true;
	}
	} else if (docTool.isSphinxFuroLikeTheme()) {
	// NOTE: The code to handle furo theme shouldn't be required,
	// since furo uses explicit placement.
	// However, the Jinja context variable READTHEDOCS is not injected anymore,
	// and furo does not includes the explicit placement due to this.
	// This is a temporal solution while they fix this.
	selector = ".sidebar-tree";
	element = document.querySelector(selector);
	if (this.elementAboveTheFold(element)) {
	placement.classList.add("ethical-alabaster");
	placement.setAttribute("data-ea-type", "readthedocs-sidebar");
	placement.setAttribute("id", "furo-sidebar-ad-placement");
	knownPlacementFound = true;
	}
	} else if (docTool.isSphinxBookThemeLikeTheme()) {
	selector = ".sidebar-primary-items__start.sidebar-primary__section";
	element = document.querySelector(selector);
	if (this.elementAboveTheFold(element)) {
	placement.classList.add("ethical-alabaster");
	placement.setAttribute("data-ea-type", "readthedocs-sidebar");
	knownPlacementFound = true;
	}
	} else if (docTool.isSphinxAlabasterLikeTheme()) {
	selector = "div.sphinxsidebar > div.sphinxsidebarwrapper";
	element = document.querySelector(selector);
	if (this.elementAboveTheFold(element)) {
	placement.classList.add("ethical-alabaster");
	placement.setAttribute("data-ea-type", "readthedocs-sidebar");
	knownPlacementFound = true;
	}
	} else if (docTool.isMaterialMkDocsTheme()) {
	selector = ".md-sidebar__scrollwrap";
	element = document.querySelector(selector);
	if (this.elementAboveTheFold(element)) {
	// TODO: use a more styled CSS class.
	// See https://github.com/readthedocs/ethical-ad-client/issues/193
	placement.classList.add("ethical-alabaster");
	placement.setAttribute("data-ea-type", "readthedocs-sidebar");
	knownPlacementFound = true;
	}
	} else if (docTool.isDocusaurusTheme()) {
	selector = ".menu.thin-scrollbar.menu_SIkG";
	element = document.querySelector(selector);
	if (this.elementAboveTheFold(element)) {
	// TODO: use a more styled CSS class.
	// See https://github.com/readthedocs/ethical-ad-client/issues/193
	placement.classList.add("ethical-alabaster");
	placement.classList.add("ethical-docusaurus");
	placement.setAttribute("data-ea-type", "readthedocs-sidebar");
	placement.setAttribute("data-ea-style", "image");
	knownPlacementFound = true;
	}
	}

We can do the same for Markdoc, mdbook and similars to have better integrations on those as well. It shouldn't be hard. We need to find the right CSS selector.

I'm not opposed to have a better default for frameworks we don't know; but we for those we are aware they exist and people are using them, we should integrate them better by using custom positioning.

NOTE: This is somewhere that it would be interesting to have a feature flag, so that we could test it a bit before rolling it out more widely.

As I mentioned in the meeting, I would not use feature flags for this, but use a AddonsConfig.ethicalads_* field instead.

[humitos]

I added an example of what I'm saying in #470. You can see it's pretty easy to create these custom positioning based on the framework. I think they will always work better (less intrusive, nicer look&feel) than a generic default.

You can try to follow those steps and add a better positioning for Markdoc, for example.

[humitos]

I'm generally becoming more excited about this placement,
especially when the floating one is pretty intrusive.

By they way, when performing visual changes, I would really appreciate having a screenshot of how it looks --that way is a lot easier to do a review; since it doesn't require me to clone the PR and testing it locally.

[agjohnson]

@humitos point on per-tool styling is probably better overall, but just noting a couple things with the approach here.

[agjohnson]

Just noting a couple issues here, I haven't tried playing around with the styling here.

Altering style rules through Element.style ends up just adding a style="..." attribute to the element, which has a couple issues:

• Inline style attributes can be blocked by CSP rules as unsafe.
• Inline style attributes have the highest specificity in CSS, so the user loses control of this styling.

In both cases, it's better to use classes to apply rules to these elements.

[agjohnson]

I just noticed this logic and the same above. These conditionals are only evaluated once, so don't catch the window resizing or rotating. Users have to reload before the placement style would shift in this case. These widths might be better off as CSS media queries, which are reactive.