Amp skimlinks exclude selector#18
Conversation
| const excludedAnchors = nodeListToArray( | ||
| this.rootNode_.querySelectorAll(this.excludeSelector_) | ||
| ); | ||
| anchors = anchors.filter(a => excludedAnchors.indexOf(a) === -1); |
There was a problem hiding this comment.
not sure how big of a deal it is, but you are not forcing the exludeSelector / linkSelector to match an actual anchor.
There was a problem hiding this comment.
It is a bit of a concern, I put a WARNING in the documentation https://amp.dev/documentation/components/amp-skimlinks/ to tell the publisher to use this option carefully. I can't think of a reliable way to validate that a CSS selector is matching an a element. Maybe some regex but not sure if we want to go down that road.
For now we need to rely on the publisher being careful.
There was a problem hiding this comment.
when you are filtering anchors you can check for .tagname but it will add some processing time (+ you will have to do the filter even if there are no excludeSelectors
| 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', |
There was a problem hiding this comment.
probably not the best idea to reference other companies here
3 participants
Uh oh!
There was an error while loading. Please reload this page.