Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
bidViewablityIO Module: add new submodule for detecting viewability without ad server dependancies #7151
bidViewablityIO Module: add new submodule for detecting viewability without ad server dependancies #7151
Changes from 2 commits
b8ff1de
2b72b39
df08062
6c4e342
60d0093
7c3a5eb
e416efd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is me being picky, but when I am testing / debugging pub pages I like when modules emit their name inside their console logs, makes it easy to filter.
You could wrap logMessage in your own function which just adds in MODULE_NAME on each message you send.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't sound picky, it sounds like an excellent practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to never register for this event until the user enables the config.
So your init function could subscribe to setConfig and then once it is enabled, it then will add the event listener.
That way the event callback is not executing every single time this module is included.
Only when it is actually turned on.
I would also add the
CLIENT_SUPPORTS_IO
check inside of that config subscribe as well since no need to run it when it is not supportedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just pushed a change that doesn't do the subscribe thing, then actually read all these words and found out that this subscribe thing is a thing. I'll have a look it doing it as you describe this afternoon probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool.
So basically if you do the following
Prebid will execute your
CALLBACK_FUNCTION
each timepbjs.setConfig
is called with yourMODULE_NAME
So for your use case, we only want to register the event listener if the user / pub runs pbjs.setConfig and turns on bidViewability.
so your callback can do something like: