Ads - specifying ad loading viewport margins for different screensizes/page types #1094
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of minor points
components/n-ui/ads/js/oAdsConfig.js
Outdated
return '0%'; | ||
function setViewportMarginBy () { | ||
let pt = appName; | ||
pt = pt.toLowerCase().substr(0, 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to substring it here? Might be more readable later on to refer to 'front-page' (or whatever the full name is)
components/n-ui/ads/js/oAdsConfig.js
Outdated
if (pt === 'fro') { | ||
if (scrnSize < 760) {return '15%';} | ||
else { | ||
if (pt === 'fro' && scrnSize < 980) {return '5%';} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking 'fro' again here shouldn't be necessary
sandbox.restore(); | ||
}); | ||
it('Should pass 15% when screen size is narrower than 760px and adOptimizeLazyLoad flag is defined and appName is fro', () => { | ||
sandbox.stub(utils, 'getScreenSize', () => { return 750; }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably remove the flag from the tests...it's a little confusing otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was planning to keep the flag but update it so that its no longer an MVT - this would allow us to toggle the viewport margin. However, i guess it doesnt give us much benefit so, as you say it might be less confusing to remove it.
@adgad @fenglish @alexflorisca