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.

Sign up for GitHub

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

Jump to bottom

Andrew s/neg vt issue #30

Merged
merged 5 commits into from
May 27, 2022
Merged

Andrew s/neg vt issue #30

merged 5 commits into from
May 27, 2022

Conversation

andrew-sotiriou
Copy link

@andrew-sotiriou andrew-sotiriou commented May 23, 2022 •
edited
Loading

What does this PR do?

Fixes Neg VT so that it only works with items that are viewable on the page

How can this change be manually tested?

local dev only

Asana Card

https://app.asana.com/0/1198498632694766/1202301331735392/f

Questions or concerns about this change?

n/a

Deployment Notes

This PR will need to be deployed first and will need to then update Oxygen repo

PR Readiness

  • Changes have been validated in local dev

Code Readiness

Automated Testing

  • Unit tests are written at the appropriate level given the code’s risk-level
  • Existing tests still pass
  • No unnecessary test runner output, e.g. debugging or warnings are not present in Jest or Unittest output

Other Considerations

  • Relevant documentation updated in Slite

Also consider this PR's impact on architecture and security; AMP, Apple News, RSS; SEO, GA, performance and accessibility.

Engywook2005
Engywook2005 reviewed May 23, 2022
View reviewed changes
src/Bling.js Outdated
};

if (inViewport && isHidden() === false) {
console.log(
Copy link

Choose a reason for hiding this comment

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

Of course we should remove this log before deployment...

Copy link
Author

Choose a reason for hiding this comment

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

yup...i missed that and just removed

Engywook2005
Engywook2005 reviewed May 23, 2022
View reviewed changes
Copy link

@Engywook2005 Engywook2005 left a comment

Choose a reason for hiding this comment

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

Other than the log, I think this looks good... just need to take that log out.

@Engywook2005
Copy link

Looks good to me and fixes the warnings.

@andrew-sotiriou andrew-sotiriou merged commit 4278140 into master May 27, 2022
@andrew-sotiriou andrew-sotiriou deleted the andrew-s/neg-vt-issue branch May 27, 2022 14:34
@andrew-sotiriou andrew-sotiriou requested review from andrewb and removed request for andrewb May 27, 2022 14:45
andrew-sotiriou added a commit that referenced this pull request Dec 18, 2023
@andrew-sotiriou
Andrew s/neg vt issue (#30)
c813c5f 
* Fixes for neg VT

* Update isHidden

* Cleanup

* Remove log

* Update call
andrew-sotiriou added a commit that referenced this pull request Jan 8, 2024
@andrew-sotiriou @carlos-algms
@miblanchard @tamagokun @andrewb @Engywook2005 @stephensauceda
Andrew s/dev week (#38)
d89e380 
* Enable non-personalized Ads (#1)

*  Enable non-personalized Ads

According to EU regulations, it is mandatory to enable users to opt-out to tracking cookies
GPT allows us to respect this law using the `npa` flag on Ads.

* Adding npa flag to re-render props

* Fix npa variable in componentWillReceiveProps

* Increase the bundle-size to accept the new amount of code

* Changed the place where NPA is set

* Update Bling.js

Fixes an issue where `setAttributes` can be called when there is no ad slot

* Bump to 2.0.2

* chore: Update package.json

Update package.json name and urls after forking

* chore: Update API list

Update GPT API list to latest

* chore: Add prettier config (#3)

Add prettier config and fix bracket spacing mismatch

* Fixing things

* fix: Don't setState on unmounted ad (#5)

Don't set state on an unmounted ad. Cribbed from
axioscode@9425436.

* fix: Fix undefined function if apiNotReady (#6)

Fix undefined function if apiNotReady. Cribbed from
ticketmaster@5ef19b1.

* fix: Replace hasOwnProperty with more reliable check (#7)

Replace hasOwnProperty with more reliable check. Cribbed from
nfl@8826ee3.

* fix: Do not refresh all ads when new component loads (#8)

Fixes an issue where all ad slots would reload when syncCorrelator was true and a new slot was added

* refactor: Check for updateCorrelator (#9)

Check for deprecated updateCorrelator method before calling

* build: Update radium (#10)

* build: Update radium

Update radium to fix "TypeError: Cannot read property 'object' of undefined" when running examples

* Use ^

* Fixing things

* fix: Stop multiple refresh on mq change (#11)

* fix: Stop multiple refresh on mq change

Stops multiple calls to instance.refresh() on media query change. This can happen when an instance
is associated with multiple instances.

* Only debounce this.refresh

* Fix comment

* Merge it

* feat: Add support for custom refresh function (#14)

Adds the ability to specify a custom refresh function using `configure`. This function replaces the
call to `pubads().refresh`. This is useful for certain header bidding configurations, such as App
Nexus that requires to `pbjs.refresh` to be called.

* work this time

* perf: Use googletag.cmd.push instead of timeout to process queue (#16)

Use googletag.cmd.push instead of timeout to check if pubads is ready. Fixes issue with recurring
handler.

* work this time

* Update all instances of googletagservices gpt url to securepubads gpt url (#20)

* Update all instances of googletagservices gpt url to securepubads gpt url

* Add UNSAFE so we can look to update REACT

* update remaining paths; use https to avoid redirect

Co-authored-by: Andrew Berry <[email protected]>

* update linking instructions; some cleanup (#21)

* remove enableServices timeout from GPT mock (#22)

* Release 2.1.2 (#24)

* Remove deprecated ContentService (#23)

* Remove deprecated ContentService usage

* Update test

* doc updates

Co-authored-by: Andrew Sotiriou <[email protected]>

* update changelog and version in package.json (#25)

* Ton of MI updates (#26)

* Ton of MI updates

* Minor changes

* change release and update changelog (#27)

* Gregt/negative viewable threshold (#28)

* Can work with negative viewability threshold.

* Offset in log remark is now VT for clarity.

* Checking for isNaN

* removed logging

Co-authored-by: Engywook2005 <[email protected]>

* Changed version and updated changelog (#29)

* Changed version and updated changelog

* Fixed typo in package.json

Co-authored-by: Engywook2005 <[email protected]>

* Andrew s/neg vt issue (#30)

* Fixes for neg VT

* Update isHidden

* Cleanup

* Remove log

* Update call

* Release 2 1 6 (#31)

* update packageJSON version

* Update changelog

* add prepare script

* Revert "add prepare script"

This reverts commit 49fcf5b.

* React v17 support (#32)

* update deps

* update radium to version supporting react 17

* add prepare script

* remove prepare script

* findDOMNode is deprecated

* make peerDependencies more lenient

* ref needs to be on different node

* Reverting the version as i will do that in another step

---------

Co-authored-by: Andrew Sotiriou <[email protected]>

* release branch (#33)

* Remove console log (#34)

* Update package and remove console log (#35)

* Gregt/lazy load (#36)

* Add stuff for lazy load

* First shot at ignoring isInViewport response.

* Will render ads even if the inviewport check fails

* Removed commented out line of code

* Version bump and changelog update

---------

Co-authored-by: Andrew Sotiriou <[email protected]>
Co-authored-by: Greg Thorson <[email protected]>

---------

Co-authored-by: Carlos A. Gomes <[email protected]>
Co-authored-by: Carlos Gomes <[email protected]>
Co-authored-by: Michael Blanchard <[email protected]>
Co-authored-by: Mike Kruk <[email protected]>
Co-authored-by: Andrew Berry <[email protected]>
Co-authored-by: Andrew Berry <[email protected]>
Co-authored-by: Greg Thorson <[email protected]>
Co-authored-by: Engywook2005 <[email protected]>
Co-authored-by: Stephen Sauceda <[email protected]>
Co-authored-by: Greg Thorson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Engywook2005 Engywook2005 Engywook2005 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andrew-sotiriou @Engywook2005