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

Conditionally load advertising manually #35

Merged
merged 1 commit into from
Oct 28, 2020

Conversation

davidfischer
Copy link
Contributor

By setting data-ea-manual, you can set ads to load manually. After that you can call ethicalads.load() to load them. This can't be used to load a new ad (eg. #24) and it will error out if it attempts to load a new ad.

By setting ``data-ea-manual``, you can set ads to load manually.
After that you can call ``ethicalads.load()`` to load them.
@davidfischer davidfischer requested review from agjohnson and a team October 8, 2020 22:44
*
* <div data-ea-publisher="..." data-ea-manual="true"></div>
* <script>
* ethicalads.load();
Copy link
Member

Choose a reason for hiding this comment

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

Does this allow us to set keywords on the request? That seems important since we don't have that data prior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably make it accept options but it doesn't currently. You can still attach the keywords to the div the normal way.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but I thought the point of this PR was to use it on RTD, where we don't know the keywords until we get the API response?

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 plan was to add/modify that DOM element after the API response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test this out and see how well it works. Maybe we do need additional options to load().

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.

Looks good 👍

@ericholscher
Copy link
Member

@davidfischer should we merge this to go out with the default client?

@ericholscher ericholscher merged commit 18ed93d into master Oct 28, 2020
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