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

Uplift #9743 from brave/news-displayads-fix-double-get #9750

Merged
merged 1 commit into from
Aug 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import useScrollIntoView from '../../useScrollIntoView'
import { useVisitDisplayAdClickHandler } from '../../useReadArticleClickHandler'
import { OnVisitDisplayAd, OnViewedDisplayAd, GetDisplayAdContent } from '../..'
import CardImage from '../CardImage'
import useTriggerOnNearViewport from './useTriggerOnNearViewport'
import * as Styles from './style'

type Props = {
Expand All @@ -22,14 +23,11 @@ type Props = {

export default function CardDisplayAd (props: Props) {
// Content is retrieved when the element is close to the viewport
// - undefined = not fetched yet
// - null = no ad available for this unit
// - DisplayAd = ad for this unit
const [content, setContent] = React.useState<BraveToday.DisplayAd | undefined | null>(undefined)
const [cardRef] = useScrollIntoView(props.shouldScrollIntoView || false)
const onClick = useVisitDisplayAdClickHandler(props.onVisitDisplayAd, content ? { ad: content } : undefined)
const innerRef = React.useRef<HTMLElement>(null)

// Setup an observer to track amount of time viewed
React.useEffect(() => {
if (!innerRef.current || !content || !props.onViewedDisplayAd) {
return
Expand All @@ -45,36 +43,17 @@ export default function CardDisplayAd (props: Props) {
}
}, [innerRef.current, props.onViewedDisplayAd, content?.uuid])
// Ask for and render the ad only when we're scrolled close to it
const contentTrigger = React.useRef<HTMLDivElement>(null)
const contentTriggerObserver = React.useRef<IntersectionObserver>()
React.useEffect(() => {
// Setup observer on first mount
contentTriggerObserver.current = new IntersectionObserver(async (entries) => {
if (entries.some((entry) => entry.isIntersecting)) {
// Get the ad and display it
const ad = await props.getContent()
// Request may not actually come back with an ad
if (ad) {
setContent(ad)
}
}
}, {
// Trigger ad fetch when the ad unit is 1000px away from the viewport
rootMargin: '0px 0px 1000px 0px'
})
})
React.useEffect(() => {
// Observe content trigger when it's appropriate.
// Don't observe (or disconnect current observer) if there is already content
if (content || !contentTrigger.current || !contentTriggerObserver.current) {
return
}
const observer = contentTriggerObserver.current
observer.observe(contentTrigger.current)
return () => {
observer.disconnect()
const handleOnNearViewport = React.useCallback(async () => {
// Get the ad and display it
const ad = await props.getContent()
// Request may not actually come back with an ad
if (ad) {
setContent(ad)
}
}, [content, contentTrigger.current, contentTriggerObserver.current])
}, [props.getContent, setContent])
const handleOnNearViewportRef = React.useRef<Function>(handleOnNearViewport)
handleOnNearViewportRef.current = handleOnNearViewport
const [contentTrigger] = useTriggerOnNearViewport(handleOnNearViewportRef)
// Render content trigger
if (!content) {
// verbose ref type conversion due to https://stackoverflow.com/questions/61102101/cannot-assign-refobjecthtmldivelement-to-refobjecthtmlelement-instance
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright (c) 2021 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// you can obtain one at http://mozilla.org/MPL/2.0/.

import * as React from 'react'

let instanceCount = 0

export default function useTriggerOnNearViewport (handlerRef: React.MutableRefObject<Function>) {
// When we're intersecting for the first time, ask for an Ad
const hasPassedAdRequestThreshold = React.useRef(false)
// Element which will be observed to trigger ad fetch
const contentTrigger = React.useRef<HTMLDivElement>(null)
// Observer which will observe elements scroll position to trigger ad fetch
const contentTriggerObserver = React.useRef<IntersectionObserver>()
const debugInstanceId = React.useMemo(function () {
return instanceCount++
}, [])
// Setup ad fetch trigger observer on first mount
React.useEffect(() => {
console.debug('Brave News: creating observer', debugInstanceId)
contentTriggerObserver.current = new IntersectionObserver(async (entries) => {
// Only request an ad the first time it passes threshold
if (!hasPassedAdRequestThreshold.current && entries.some((entry) => entry.isIntersecting)) {
hasPassedAdRequestThreshold.current = true
console.debug('Brave News: asking for an ad', debugInstanceId)
handlerRef.current && handlerRef.current()
}
}, {
// Trigger ad fetch when the ad unit is 1000px away from the viewport
rootMargin: '0px 0px 1000px 0px'
})
}, [])
// Observe and disconnect from content trigger when it's appropriate
React.useEffect(() => {
if (hasPassedAdRequestThreshold.current || !contentTrigger.current || !contentTriggerObserver.current) {
console.debug('Brave News: not creating trigger', debugInstanceId)
return
}
const observer = contentTriggerObserver.current
console.debug('Brave News: ad fetch trigger observer connected', debugInstanceId)
observer.observe(contentTrigger.current)
return () => {
console.debug('Brave News: ad fetch trigger observer disconnected', debugInstanceId)
observer.disconnect()
}
}, [
// Disconnect when we have asked for an ad
// (opportunistically at the next render as ref mutations don't cause effects to run)
hasPassedAdRequestThreshold.current,
// Observer or Disconnect when we have a new element (should have caused a render, so this should get run)
contentTrigger.current,
// Observe or Disconnect when we have a new intersection observer (should be at first mount)
contentTriggerObserver.current
])
return [contentTrigger]
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ let callCount = 0

export default function getBraveNewsDisplayAd (always: boolean = false) {
callCount++
const ad = (always || callCount % 2)
const ad: BraveToday.DisplayAd | null = (always || callCount % 2)
? {
description: 'Technica',
uuid: '0abc',
Expand All @@ -19,5 +19,7 @@ export default function getBraveNewsDisplayAd (always: boolean = false) {
dimensions: '1x3'
}
: null
return Promise.resolve(ad)
return new Promise<BraveToday.DisplayAd | null>(function (resolve) {
setTimeout(() => resolve(ad), 2400)
})
}