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

EthicalAds: Default to fixedfooter for smaller screens #447

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Prev Previous commit
Next Next commit
Note it's a hack
ericholscher committed Dec 11, 2024
commit a4ef3abb4f21e2c9d1cc2a3b8c5001c77af071b2
1 change: 1 addition & 0 deletions src/ethicalads.js
Original file line number Diff line number Diff line change
@@ -162,6 +162,7 @@ export class EthicalAdsAddon extends AddonBase {
// Use fixed footer for smaller widths, but not mobile
placement.setAttribute("data-ea-type", "text");
placement.setAttribute("data-ea-style", "fixedfooter");
// TODO: THIS IS A HACK. IS THERE A BETTER WAY?
// Add margin to the bottom to avoid hiding bottom of content
const root_node = document.querySelector(docTool.getRootSelector());
root_node.style.marginBottom = "2em";
Copy link
Contributor

Choose a reason for hiding this comment

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

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.