Skip to content

Add a fixedheader ad style #218

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Feb 20, 2025

  • Pinned Ad type at the top of the page. Supports text-only or image+text, but hidden on mobile. It is not pinned. Users can just scroll past it.
  • Much heavier use of CSS variables
  • Because there's some duplication in defining colors twice, this does add a bit of size ~8kb minified to the ad client. I'm leaning toward a separate PR that makes all the colors into variables. That should make the minified ad client a bit smaller as it'll remove the duplication.

- Pinned ad type at the top of the page.
  Supports text-only or image+text, but hidden on mobile.
- Much heavier use of CSS variables
- Because there's some duplication in defining colors twice,
  this does add a bit of size ~8kb minified to the ad client.
@davidfischer davidfischer requested review from agjohnson and a team February 20, 2025 21:10
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.

Would probably be useful to get @agjohnson review on the CSS as well, but I took a quick look at it.


<!-- Place this div just after the open <body> tag (top of the document) -->
<!-- You can also use `data-ea-type="text"` for a text-only ad -->
<!-- Set style="height 50px" to preallocate space for the ad -->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- Set style="height 50px" to preallocate space for the ad -->
<!-- Set style="height: 50px" to preallocate space for the ad -->

I think this is right. But we should probably include this in the default example below, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, maybe mention that it should either be here or via CSS.

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.

2 participants