Skip to content

Commit

Permalink
Libs tidy up pt.3 (#1721)
Browse files Browse the repository at this point in the history
* inline spacefinder utils

* Inline and simplify get-ad-iframe

* Delete unused geolocation functions
  • Loading branch information
arelra authored Jan 6, 2025
1 parent cdefafe commit ccd5e61
Show file tree
Hide file tree
Showing 10 changed files with 46 additions and 421 deletions.
4 changes: 1 addition & 3 deletions src/core/targeting/youtube-ima.spec.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import type { ConsentState } from '@guardian/libs';
import * as BuildPageTargeting from './build-page-targeting';
import { buildPageTargeting } from './build-page-targeting';
import { buildImaAdTagUrl } from './youtube-ima';

const { buildPageTargeting } = BuildPageTargeting;

jest.mock('./build-page-targeting', () => ({
// we want the real filterValues()
...jest.requireActual('./build-page-targeting'),
Expand Down
2 changes: 1 addition & 1 deletion src/display/request-bids.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const retainAdSizeOnRefresh = (
* passed in that have already had bids requested, this can happen if they're already in
* the viewport, it will only request bids for adverts that haven't already had bids requested.
*/
export const requestBidsForAds = async (adverts: Advert[]): Promise<void> => {
const requestBidsForAds = async (adverts: Advert[]): Promise<void> => {
const adsToRequestBidsFor = adverts.filter(
(advert) => !advert.headerBiddingBidRequest,
);
Expand Down
18 changes: 16 additions & 2 deletions src/events/render-advert.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { adSizes } from '../core/ad-sizes';
import type { Advert } from '../define/Advert';
import { getAdIframe } from '../lib/dfp/get-ad-iframe';
import { $$ } from '../utils/$$';
import fastdom from '../utils/fastdom-promise';
import { reportError } from '../utils/report-error';
Expand Down Expand Up @@ -179,6 +178,21 @@ const addContainerClass = (adSlotNode: HTMLElement, isRendered: boolean) => {
});
};

/**
* Check if the ad slot has a corresponding iframe to indicte the ad has rendered.
* @param adSlot
* @returns
*/
const hasIframe = (adSlot: HTMLElement): Promise<boolean> =>
new Promise((resolve) => {
// DFP will sometimes return empty iframes, denoted with a '__hidden__' parameter embedded in its ID.
// We need to be sure only to select the ad content frame.
const iFrame = adSlot.querySelector<HTMLIFrameElement>(
'iframe:not([id*="__hidden__"])',
);
resolve(!!iFrame);
});

/**
* @param advert - as defined in lib/dfp/Advert
* @param slotRenderEndedEvent - GPT slotRenderEndedEvent
Expand All @@ -189,7 +203,7 @@ const renderAdvert = (
slotRenderEndedEvent: googletag.events.SlotRenderEndedEvent,
): Promise<boolean> => {
addContentClass(advert.node);
return getAdIframe(advert.node)
return hasIframe(advert.node)
.then((isRendered) => {
const creativeTemplateId =
slotRenderEndedEvent.creativeTemplateId ?? undefined;
Expand Down
6 changes: 4 additions & 2 deletions src/init/consented/prepare-googletag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ jest.mock('display/display-lazy-ads', () => ({
displayLazyAds: jest.fn(),
}));

jest.mock('core/ad-sizes', () => {
const adSizes: typeof AdSizesType = jest.requireActual('core/ad-sizes');
jest.mock('../../core/ad-sizes', () => {
const adSizes: typeof AdSizesType = jest.requireActual(
'../../core/ad-sizes',
);
const { createAdSize } = adSizes;
return {
...adSizes,
Expand Down
2 changes: 1 addition & 1 deletion src/init/consented/static-ad-slots.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const decideAdditionalSizes = (adSlot: HTMLElement): SizeMapping => {
/**
* Static ad slots that were rendered on the page by the server are collected here.
*
* For dynamic ad slots that are created at js-runtime, see:
* For dynamic ad slots that are created at runtime, see:
* - article-aside-adverts
* - article-body-adverts
* - high-merch
Expand Down
18 changes: 17 additions & 1 deletion src/insert/spacefinder/rules.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,27 @@
import { adSlotContainerClass } from '../../core/create-ad-slot';
import { adSizes } from '../../core/index';
import type { OpponentSelectorRules, SpacefinderRules } from './spacefinder';
import { isInHighValueSection } from './utils';

const bodySelector = '.article-body-commercial-selector';
const adSlotContainerSelector = `.${adSlotContainerClass}`;

const highValueSections = [
'business',
'environment',
'music',
'money',
'artanddesign',
'science',
'stage',
'travel',
'wellness',
'games',
];

const isInHighValueSection = highValueSections.includes(
window.guardian.config.page.section,
);

/**
* As estimation of the height of the most viewed island.
* This appears from desktop breakpoints on the right-hand side.
Expand Down
18 changes: 0 additions & 18 deletions src/insert/spacefinder/utils.ts

This file was deleted.

46 changes: 0 additions & 46 deletions src/lib/dfp/get-ad-iframe.ts

This file was deleted.

13 changes: 5 additions & 8 deletions src/utils/geo-utils.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { CountryCode } from '@guardian/libs';
import { setCookie } from '@guardian/libs';
import {
_,
isInAuOrNz,
Expand All @@ -11,16 +11,10 @@ import {
isInUsOrCa,
} from './geo-utils';

let mockCountryCode: CountryCode;
jest.mock('utils/geolocation', () => ({
getCountryCode: jest.fn(() => mockCountryCode),
}));

describe('Geolocation Utils', () => {
beforeEach(() => {
_.resetModule();
});

const testCases = [
{
fnName: 'isInUk()',
Expand Down Expand Up @@ -98,7 +92,10 @@ describe('Geolocation Utils', () => {

testCases.forEach((testCase) => {
it(`Only ${testCase.fnName} return true for geolocation '${testCase.mockCountryCode}'`, () => {
mockCountryCode = testCase.mockCountryCode;
setCookie({
name: 'GU_geo_country',
value: testCase.mockCountryCode,
});
expect(isInUk()).toBe(testCase.expectedUKValue);
expect(isInUsa()).toBe(testCase.expectedUsaValue);
expect(isInCanada()).toBe(testCase.expectedCaValue);
Expand Down
Loading

0 comments on commit ccd5e61

Please sign in to comment.