From a4910cae9bd1b1da66227e83627fb16245ae6242 Mon Sep 17 00:00:00 2001 From: Vincent Comby Date: Mon, 2 Dec 2019 12:43:38 +0100 Subject: [PATCH 1/7] Add tests for new linkValidator option --- .../0.1/test/test-link-rewriter.js | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/extensions/amp-skimlinks/0.1/test/test-link-rewriter.js b/extensions/amp-skimlinks/0.1/test/test-link-rewriter.js index 9e63ddd1f31c..f9ab8f754419 100644 --- a/extensions/amp-skimlinks/0.1/test/test-link-rewriter.js +++ b/extensions/amp-skimlinks/0.1/test/test-link-rewriter.js @@ -590,6 +590,52 @@ describes.fakeWin('Link Rewriter', {amp: true}, env => { expect(resolveFunction.firstCall.args[0]).to.deep.equal([anchor2]); }); + + it('Should ignore links matching excludeSelector', () => { + const anchor1 = rootDocument.createElement('a'); + const anchor2 = rootDocument.createElement('a'); + anchor2.classList.add('no-affiliate'); + const anchor3 = rootDocument.createElement('a'); + + rootDocument.body.appendChild(anchor1); + rootDocument.body.appendChild(anchor2); + rootDocument.body.appendChild(anchor3); + + const resolveFunction = createResolveResponseHelper(); + createLinkRewriterHelper(resolveFunction, { + excludeSelector: 'a.no-affiliate', + }).scanLinksOnPage_(); + + expect(resolveFunction.firstCall.args[0]).to.deep.equal([ + anchor1, + anchor3, + ]); + }); + + it('excludeSelector should have priority over linkSelector', () => { + const anchor1 = rootDocument.createElement('a'); + anchor1.classList.add('affiliate'); + const anchor2 = rootDocument.createElement('a'); + anchor2.classList.add('affiliate'); + const anchor3 = rootDocument.createElement('a'); + + const noskimContainer = rootDocument.createElement('div'); + noskimContainer.classList.add('no-affiliate'); + + rootDocument.body.appendChild(noskimContainer); + // Anchor1 has affiliate class but is inside a container with no-affiliate. + noskimContainer.appendChild(anchor1); + rootDocument.body.appendChild(anchor2); + rootDocument.body.appendChild(anchor3); + + const resolveFunction = createResolveResponseHelper(); + createLinkRewriterHelper(resolveFunction, { + linkSelector: 'a.affiliate', + excludeSelector: '.no-affiliate a', + }).scanLinksOnPage_(); + + expect(resolveFunction.firstCall.args[0]).to.deep.equal([anchor2]); + }); }); describe('In dynamic page', () => { From 609f7211e7f924fb2647136f55fb00cbda47a786 Mon Sep 17 00:00:00 2001 From: Vincent Comby Date: Mon, 2 Dec 2019 12:51:00 +0100 Subject: [PATCH 2/7] Create new exclude-selector attribute --- extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html | 3 ++- extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out | 3 ++- extensions/amp-skimlinks/validator-amp-skimlinks.protoascii | 3 +++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html index 2b94272051a1..91d7ee07c0ef 100644 --- a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html +++ b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html @@ -29,7 +29,8 @@ layout="nodisplay" publisher-code="123X123" excluded-domains="samsung.com asos.com" - link-selector="article:not(.no-skimlinks) a" + link-selector="article a" + exclude-selector=".no-affiliate a" custom-tracking-id="phones" custom-redirect-domain="go.publisher.com" > diff --git a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out index e6535de68321..6cf46759a5e9 100644 --- a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out +++ b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out @@ -30,7 +30,8 @@ PASS | layout="nodisplay" | publisher-code="123X123" | excluded-domains="samsung.com asos.com" -| link-selector="article:not(.no-skimlinks) a" +| link-selector="article a" +| exclude-selector=".no-affiliate a" | custom-tracking-id="phones" | custom-redirect-domain="go.publisher.com" | > diff --git a/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii b/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii index 16e7fab7fa2d..b79a68674d8d 100644 --- a/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii +++ b/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii @@ -45,6 +45,9 @@ tags: { # attrs: { name: "link-selector" } + attrs: { + name: "exclude-selector" + } attrs: { name: "publisher-code" value_regex_casei: "^[0-9]+X[0-9]+$" From e029c056bf4cbec24046d9b204628cfacfe50dc4 Mon Sep 17 00:00:00 2001 From: Vincent Comby Date: Mon, 2 Dec 2019 15:21:24 +0100 Subject: [PATCH 3/7] Add excludeSelector logic --- extensions/amp-skimlinks/0.1/amp-skimlinks.js | 1 + .../link-rewriter/link-rewriter-manager.js | 8 +--- .../0.1/link-rewriter/link-rewriter.js | 41 ++++++++++++++++--- extensions/amp-skimlinks/0.1/skim-options.js | 12 ++++++ extensions/amp-skimlinks/0.1/utils.js | 9 ++++ 5 files changed, 58 insertions(+), 13 deletions(-) diff --git a/extensions/amp-skimlinks/0.1/amp-skimlinks.js b/extensions/amp-skimlinks/0.1/amp-skimlinks.js index a239c7e9a528..5042ba896cd6 100644 --- a/extensions/amp-skimlinks/0.1/amp-skimlinks.js +++ b/extensions/amp-skimlinks/0.1/amp-skimlinks.js @@ -161,6 +161,7 @@ export class AmpSkimlinks extends AMP.BaseElement { initSkimlinksLinkRewriter_() { const options = { linkSelector: this.skimOptions_.linkSelector, + excludeSelector: this.skimOptions_.excludeSelector, }; const linkRewriter = this.linkRewriterService_.registerLinkRewriter( diff --git a/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter-manager.js b/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter-manager.js index f58e1fcde27c..b474ba43807e 100644 --- a/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter-manager.js +++ b/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter-manager.js @@ -90,13 +90,7 @@ export class LinkRewriterManager { * @param {!function(!Array): !./two-steps-response.TwoStepsResponse} resolveUnknownLinks * - Function to determine which anchor should be replaced and by what URL. * Should return an instance of './two-steps-response.TwoStepsResponse'. - * @param {?{linkSelector: string}=} options - * - linkSelector is an optional CSS selector to restrict - * which anchors the link rewriter should handle. - * Anchors not matching the CSS selector will be ignored. - * If not provided the link rewrite will handle all the links - * found on the page. - * + * @param {?./link-rewriter.LinkRewriterOptions=} options * @return {!./link-rewriter.LinkRewriter} * @public */ diff --git a/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter.js b/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter.js index 673eccf6fb79..ffc03981736f 100644 --- a/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter.js +++ b/extensions/amp-skimlinks/0.1/link-rewriter/link-rewriter.js @@ -19,11 +19,18 @@ import {EVENTS, ORIGINAL_URL_ATTRIBUTE} from './constants'; import {LinkReplacementCache} from './link-replacement-cache'; import {Observable} from '../../../../src/observable'; import {TwoStepsResponse} from './two-steps-response'; +import {nodeListToArray} from '../utils'; import {userAssert} from '../../../../src/log'; /** @typedef {!Array<{anchor: !HTMLElement, replacementUrl: ?string}>}} */ export let AnchorReplacementList; +/** @typedef {{ + * linkSelector: (string|undefined), + * excludeSelector: (string|undefined) + * }} */ +export let LinkRewriterOptions; + /** * LinkRewriter works together with LinkRewriterManager to allow rewriting * links at click time. E.g: Replacing a link by its affiliate version only if @@ -47,7 +54,14 @@ export class LinkRewriter { * @param {!Document|!ShadowRoot} rootNode * @param {string} id * @param {function(!Array):!TwoStepsResponse} resolveUnknownLinks - * @param {?{linkSelector: string}=} options + * @param {?LinkRewriterOptions=} options + * - "linkSelector" option is an optional CSS selector to restrict + * which anchors the link rewriter should handle. + * Only anchors matching the CSS selector will be considered for affiliate. + * If not provided the link rewrite will handle all the links + * found on the page. + * - "excludeSelector" option is an optional CSS selector. All anchors + * matching this selector will be ignored by amp-skimlinks. */ constructor(rootNode, id, resolveUnknownLinks, options) { /** @public {!../../../../src/observable.Observable} */ @@ -65,6 +79,9 @@ export class LinkRewriter { /** @private {string} */ this.linkSelector_ = options.linkSelector || 'a'; + /** @private {string} */ + this.excludeSelector_ = options.excludeSelector || ''; + /** @private {number} */ this.restoreDelay_ = 300; //ms @@ -188,8 +205,8 @@ export class LinkRewriter { // Register all new anchors discovered as "unknown" status. // Note: Only anchors with a status will be considered in the click - // handlers. (Other anchors are assumed to be the ones exluded by - // linkSelector_) + // handlers. (Other anchors are assumed to be the ones excluded by + // linkSelector_ and excludeSelector_) this.anchorReplacementCache_.updateReplacementUrls( unknownAnchors.map(anchor => ({anchor, replacementUrl: null})) ); @@ -236,12 +253,24 @@ export class LinkRewriter { /** * Get the list of anchors element in the page. - * (Based on linkSelector option) + * (Based on linkSelector & excludeSelector options) * @return {!Array} * @private */ getLinksInDOM_() { - const q = this.rootNode_.querySelectorAll(this.linkSelector_); - return [].slice.call(q); + const includedNodeList = this.rootNode_.querySelectorAll( + this.linkSelector_ + ); + // Convert from NodeList to array + let anchors = nodeListToArray(includedNodeList); + + if (this.excludeSelector_) { + const excludedAnchors = nodeListToArray( + this.rootNode_.querySelectorAll(this.excludeSelector_) + ); + anchors = anchors.filter(a => excludedAnchors.indexOf(a) === -1); + } + + return anchors; } } diff --git a/extensions/amp-skimlinks/0.1/skim-options.js b/extensions/amp-skimlinks/0.1/skim-options.js index 795b6633f812..57446675716b 100644 --- a/extensions/amp-skimlinks/0.1/skim-options.js +++ b/extensions/amp-skimlinks/0.1/skim-options.js @@ -47,6 +47,7 @@ export function getAmpSkimlinksOptions(element, docInfo) { tracking: getTrackingStatus_(element), customTrackingId: getCustomTrackingId_(element), linkSelector: getLinkSelector_(element), + excludeSelector: getExcludeSelector_(element), waypointBaseUrl: getWaypointBaseUrl(element), config: getConfig_(element), }; @@ -131,6 +132,17 @@ function getLinkSelector_(element) { return linkSelector || null; } +/** + * + * @param {!Element} element + * @return {?string} + */ +function getExcludeSelector_(element) { + const linkSelector = element.getAttribute('exclude-selector'); + + return linkSelector || null; +} + /** * * @param {?../../../src/service/document-info-impl.DocumentInfoDef} docInfo diff --git a/extensions/amp-skimlinks/0.1/utils.js b/extensions/amp-skimlinks/0.1/utils.js index c8decfd71bf0..678c571715f5 100644 --- a/extensions/amp-skimlinks/0.1/utils.js +++ b/extensions/amp-skimlinks/0.1/utils.js @@ -78,3 +78,12 @@ export function isExcludedAnchorUrl(anchor, skimOptions) { const domain = getNormalizedHostnameFromAnchor(anchor); return isExcludedDomain(domain, skimOptions); } + +/** + * Convert a NodeList to an Array. + * @param {NodeList} nodeList + * @return {!Array} + */ +export function nodeListToArray(nodeList) { + return [].slice.call(nodeList); +} From 2ce37b21442a825384784c0c204de3855fe1c68c Mon Sep 17 00:00:00 2001 From: Vincent Comby Date: Mon, 2 Dec 2019 15:48:34 +0100 Subject: [PATCH 4/7] Add default exclude of the "noskimlinks" class. --- extensions/amp-skimlinks/0.1/skim-options.js | 13 +++++++--- .../0.1/test/test-skim-options.js | 25 +++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/extensions/amp-skimlinks/0.1/skim-options.js b/extensions/amp-skimlinks/0.1/skim-options.js index 57446675716b..e56a01c17e87 100644 --- a/extensions/amp-skimlinks/0.1/skim-options.js +++ b/extensions/amp-skimlinks/0.1/skim-options.js @@ -138,9 +138,16 @@ function getLinkSelector_(element) { * @return {?string} */ function getExcludeSelector_(element) { - const linkSelector = element.getAttribute('exclude-selector'); - - return linkSelector || null; + // Always exclude: + // - links with "noskimlinks" class + // - links within a parent container (direct or not) with a "noskimlinks" class. + const defaultExcludeSelector = 'a.noskimlinks, .noskimlinks a'; + const userExcludeSelector = element.getAttribute('exclude-selector'); + const excludeSelector = userExcludeSelector + ? userExcludeSelector.concat(`, ${defaultExcludeSelector}`) + : defaultExcludeSelector; + + return excludeSelector; } /** diff --git a/extensions/amp-skimlinks/0.1/test/test-skim-options.js b/extensions/amp-skimlinks/0.1/test/test-skim-options.js index b97358cfa09b..19c4ceeb67d5 100644 --- a/extensions/amp-skimlinks/0.1/test/test-skim-options.js +++ b/extensions/amp-skimlinks/0.1/test/test-skim-options.js @@ -141,5 +141,30 @@ describes.fakeWin( expect(options.waypointBaseUrl).to.equal(`http://${cname}`); }); }); + + describe('exclude-selector', () => { + it('Should have the noskimlinks exclude selector by default', () => { + const element = helpers.createAmpSkimlinksElement({ + 'publisher-code': '123X123', + }); + const options = getAmpSkimlinksOptions(element, docInfo); + + expect(options.excludeSelector).to.equal( + 'a.noskimlinks, .noskimlinks a' + ); + }); + + it('Should have both the noskimlinks exclude selector and the custom exclude selector when defined', () => { + const element = helpers.createAmpSkimlinksElement({ + 'publisher-code': '123X123', + 'exclude-selector': '.taboola a', + }); + const options = getAmpSkimlinksOptions(element, docInfo); + + expect(options.excludeSelector).to.equal( + '.taboola a, a.noskimlinks, .noskimlinks a' + ); + }); + }); } ); From 68f91b87c25b9cc8a6a9be41b3fe868384fc03f4 Mon Sep 17 00:00:00 2001 From: Vincent Comby Date: Tue, 3 Dec 2019 09:42:03 +0100 Subject: [PATCH 5/7] Create include-selector property to replace link-selector --- extensions/amp-skimlinks/0.1/amp-skimlinks.js | 2 +- extensions/amp-skimlinks/0.1/skim-options.js | 13 +++--- .../0.1/test/test-amp-skimlinks.js | 8 ++-- .../0.1/test/test-skim-options.js | 44 +++++++++++++++++++ .../0.1/test/validator-amp-skimlinks.html | 1 + .../0.1/test/validator-amp-skimlinks.out | 1 + .../validator-amp-skimlinks.protoascii | 3 ++ 7 files changed, 62 insertions(+), 10 deletions(-) diff --git a/extensions/amp-skimlinks/0.1/amp-skimlinks.js b/extensions/amp-skimlinks/0.1/amp-skimlinks.js index 5042ba896cd6..a5853dbfc5d8 100644 --- a/extensions/amp-skimlinks/0.1/amp-skimlinks.js +++ b/extensions/amp-skimlinks/0.1/amp-skimlinks.js @@ -160,7 +160,7 @@ export class AmpSkimlinks extends AMP.BaseElement { */ initSkimlinksLinkRewriter_() { const options = { - linkSelector: this.skimOptions_.linkSelector, + linkSelector: this.skimOptions_.includeSelector, excludeSelector: this.skimOptions_.excludeSelector, }; diff --git a/extensions/amp-skimlinks/0.1/skim-options.js b/extensions/amp-skimlinks/0.1/skim-options.js index e56a01c17e87..f39e011cd4d5 100644 --- a/extensions/amp-skimlinks/0.1/skim-options.js +++ b/extensions/amp-skimlinks/0.1/skim-options.js @@ -46,7 +46,7 @@ export function getAmpSkimlinksOptions(element, docInfo) { excludedDomains: getExcludedDomains_(element, getInternalDomains_(docInfo)), tracking: getTrackingStatus_(element), customTrackingId: getCustomTrackingId_(element), - linkSelector: getLinkSelector_(element), + includeSelector: getIncludeSelector_(element), excludeSelector: getExcludeSelector_(element), waypointBaseUrl: getWaypointBaseUrl(element), config: getConfig_(element), @@ -122,14 +122,17 @@ function getCustomTrackingId_(element) { } /** - * * @param {!Element} element * @return {?string} */ -function getLinkSelector_(element) { - const linkSelector = element.getAttribute('link-selector'); +function getIncludeSelector_(element) { + let includeSelector = element.getAttribute('include-selector'); + if (!includeSelector) { + // 'link-selector' is a deprecated equivalent of 'include-selector'. + includeSelector = element.getAttribute('link-selector'); + } - return linkSelector || null; + return includeSelector || null; } /** diff --git a/extensions/amp-skimlinks/0.1/test/test-amp-skimlinks.js b/extensions/amp-skimlinks/0.1/test/test-amp-skimlinks.js index b1c5bfc41c4f..6de426173db2 100644 --- a/extensions/amp-skimlinks/0.1/test/test-amp-skimlinks.js +++ b/extensions/amp-skimlinks/0.1/test/test-amp-skimlinks.js @@ -95,7 +95,7 @@ describes.fakeWin( 'excluded-domains': 'amazon.com amazon.fr ', tracking: false, 'custom-tracking-id': 'campaignX', - 'link-selector': '.content a', + 'include-selector': '.content a', }; ampSkimlinks = helpers.createAmpSkimlinks(options); env.sandbox.stub(ampSkimlinks, 'startSkimcore_'); @@ -107,7 +107,7 @@ describes.fakeWin( pubcode: options['publisher-code'], tracking: options['tracking'], customTrackingId: options['custom-tracking-id'], - linkSelector: options['link-selector'], + includeSelector: options['include-selector'], }); expect(ampSkimlinks.skimOptions_.excludedDomains).to.include.members([ 'amazon.com', @@ -121,7 +121,7 @@ describes.fakeWin( beforeEach(() => { ampSkimlinks.skimOptions_ = { - linkSelector: '.article a', + includeSelector: '.article a', }; ampSkimlinks.linkRewriterService_ = new LinkRewriterManager( env.ampdoc @@ -152,7 +152,7 @@ describes.fakeWin( expect(args[0]).to.equal(SKIMLINKS_REWRITER_ID); expect(args[1]).to.be.a('function'); expect(args[2].linkSelector).to.equal( - ampSkimlinks.skimOptions_.linkSelector + ampSkimlinks.skimOptions_.includeSelector ); }); diff --git a/extensions/amp-skimlinks/0.1/test/test-skim-options.js b/extensions/amp-skimlinks/0.1/test/test-skim-options.js index 19c4ceeb67d5..d7958b64efdb 100644 --- a/extensions/amp-skimlinks/0.1/test/test-skim-options.js +++ b/extensions/amp-skimlinks/0.1/test/test-skim-options.js @@ -142,6 +142,50 @@ describes.fakeWin( }); }); + describe('include-selector', () => { + it('Should be null by default', () => { + const element = helpers.createAmpSkimlinksElement({ + 'publisher-code': '123X123', + }); + const options = getAmpSkimlinksOptions(element, docInfo); + + expect(options.includeSelector).to.be.null; + }); + + it('Should read the "include-selector" option', () => { + const element = helpers.createAmpSkimlinksElement({ + 'publisher-code': '123X123', + 'include-selector': 'article a', + }); + const options = getAmpSkimlinksOptions(element, docInfo); + + expect(options.includeSelector).to.equal('article a'); + }); + + it('Should read deprecated "link-selector" option', () => { + const element = helpers.createAmpSkimlinksElement({ + 'publisher-code': '123X123', + // legacy equivalent of 'include-selector' + 'link-selector': 'article a', + }); + const options = getAmpSkimlinksOptions(element, docInfo); + + expect(options.includeSelector).to.equal('article a'); + }); + + it('Should prioritise "include-selector" over "link-selector" option', () => { + const element = helpers.createAmpSkimlinksElement({ + 'publisher-code': '123X123', + 'include-selector': 'article a', + // legacy equivalent of 'include-selector' + 'link-selector': 'article.press a', + }); + const options = getAmpSkimlinksOptions(element, docInfo); + + expect(options.includeSelector).to.equal('article a'); + }); + }); + describe('exclude-selector', () => { it('Should have the noskimlinks exclude selector by default', () => { const element = helpers.createAmpSkimlinksElement({ diff --git a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html index 91d7ee07c0ef..fc8c870c13bd 100644 --- a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html +++ b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.html @@ -30,6 +30,7 @@ publisher-code="123X123" excluded-domains="samsung.com asos.com" link-selector="article a" + include-selector="article a" exclude-selector=".no-affiliate a" custom-tracking-id="phones" custom-redirect-domain="go.publisher.com" diff --git a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out index 6cf46759a5e9..53daaba1aa7e 100644 --- a/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out +++ b/extensions/amp-skimlinks/0.1/test/validator-amp-skimlinks.out @@ -31,6 +31,7 @@ PASS | publisher-code="123X123" | excluded-domains="samsung.com asos.com" | link-selector="article a" +| include-selector="article a" | exclude-selector=".no-affiliate a" | custom-tracking-id="phones" | custom-redirect-domain="go.publisher.com" diff --git a/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii b/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii index b79a68674d8d..0fee63afcc99 100644 --- a/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii +++ b/extensions/amp-skimlinks/validator-amp-skimlinks.protoascii @@ -45,6 +45,9 @@ tags: { # attrs: { name: "link-selector" } + attrs: { + name: "include-selector" + } attrs: { name: "exclude-selector" } From e097ed384a7527015f605e4d3479ba4a39c02836 Mon Sep 17 00:00:00 2001 From: Vincent Comby Date: Thu, 5 Dec 2019 14:27:27 +0100 Subject: [PATCH 6/7] Change selector --- extensions/amp-skimlinks/0.1/test/test-skim-options.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/amp-skimlinks/0.1/test/test-skim-options.js b/extensions/amp-skimlinks/0.1/test/test-skim-options.js index d7958b64efdb..3beae547d95c 100644 --- a/extensions/amp-skimlinks/0.1/test/test-skim-options.js +++ b/extensions/amp-skimlinks/0.1/test/test-skim-options.js @@ -201,12 +201,12 @@ describes.fakeWin( it('Should have both the noskimlinks exclude selector and the custom exclude selector when defined', () => { const element = helpers.createAmpSkimlinksElement({ 'publisher-code': '123X123', - 'exclude-selector': '.taboola a', + 'exclude-selector': '.no-affiliate a', }); const options = getAmpSkimlinksOptions(element, docInfo); expect(options.excludeSelector).to.equal( - '.taboola a, a.noskimlinks, .noskimlinks a' + '.no-affiliate a, a.noskimlinks, .noskimlinks a' ); }); }); From 0c529cbfde1db94a11b8d5bd94e07b13d94d6190 Mon Sep 17 00:00:00 2001 From: Vincent Comby Date: Thu, 5 Dec 2019 15:12:02 +0100 Subject: [PATCH 7/7] Fix punctuation --- extensions/amp-skimlinks/0.1/skim-options.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/extensions/amp-skimlinks/0.1/skim-options.js b/extensions/amp-skimlinks/0.1/skim-options.js index f39e011cd4d5..9b2b74731e89 100644 --- a/extensions/amp-skimlinks/0.1/skim-options.js +++ b/extensions/amp-skimlinks/0.1/skim-options.js @@ -90,7 +90,6 @@ function getPubCode_(element) { } /** - * * @param {!Element} element * @return {boolean} */ @@ -142,8 +141,8 @@ function getIncludeSelector_(element) { */ function getExcludeSelector_(element) { // Always exclude: - // - links with "noskimlinks" class - // - links within a parent container (direct or not) with a "noskimlinks" class. + // - Links with "noskimlinks" class. + // - Links within a parent container (direct or not) with a "noskimlinks" class. const defaultExcludeSelector = 'a.noskimlinks, .noskimlinks a'; const userExcludeSelector = element.getAttribute('exclude-selector'); const excludeSelector = userExcludeSelector