馃悰 Add 'no-fill' attribute from AdsenseNetworkConfig. #22956
Conversation
aishwaryavora
commented
Jun 21, 2019
aishwaryavora
commented
- Adding new method in AdsenseNetworkConfig to get sticky ads attributes like 'no-fill'.
- Updating anchor ad strategy to add sticky ad attributes on amp ad element.
| /** | ||
| * Returns the sticky ad attributes that should be applied on sticky ad. | ||
| * @param {!JsonObject} unusedConfigObj | ||
| * @return {JsonObject<string, string>} null if there is no sticky ad attributes. |
There was a problem hiding this comment.
Why do you need all of this here? Instead can you put attribute extraction for sticky ads solely within AnchorAdStrategy? All you need is the config which is accessible there.
There was a problem hiding this comment.
'no-fill' attribute is specifically related to Adsense network so to keep it under Adsense network config, I moved logic here. Also making it in more generalise way so in the future if any other attributes that we might want to apply on sticky ads can be under Adsense config. It will avoid isAdsense check at other places.
There was a problem hiding this comment.
If we want this logic to live client-side, then I think what @aishwaryavora is doing here is the right approach. We are introducing the concept of a non-filled sticky ad request and also a data-no-fill attribute. Since this (at the moment) is an AdSense only concepts, it makes sense to keep the logic for it in the AdSenseNetworkConfig and not pollute the generalized amp-auto-ads code with isAdsense checks.
Another approach that was discussed, would be to just add a new field in the config that contains attributes that are specific to the anchor ad strategy. We would then not require the new OptInStatus or any of this logic client-side. All the decision logic about whether to enable anchor and whether a data-no-fill attribute should be used could then live in the ad server.
| /** | ||
| * Returns the sticky ad attributes that should be applied on sticky ad. | ||
| * @param {!JsonObject} unusedConfigObj | ||
| * @return {JsonObject<string, string>} null if there is no sticky ad attributes. |
There was a problem hiding this comment.
If we want this logic to live client-side, then I think what @aishwaryavora is doing here is the right approach. We are introducing the concept of a non-filled sticky ad request and also a data-no-fill attribute. Since this (at the moment) is an AdSense only concepts, it makes sense to keep the logic for it in the AdSenseNetworkConfig and not pollute the generalized amp-auto-ads code with isAdsense checks.
Another approach that was discussed, would be to just add a new field in the config that contains attributes that are specific to the anchor ad strategy. We would then not require the new OptInStatus or any of this logic client-side. All the decision logic about whether to enable anchor and whether a data-no-fill attribute should be used could then live in the ad server.
| if (!isObject(configObj['attributes']) || isArray(configObj['attributes'])) { | ||
| user().warn(TAG, 'attributes property not an object'); | ||
| if (!isObject(configObj[attributes]) || isArray(configObj[attributes])) { | ||
| user().warn(TAG, attributes + ' property not an object'); |
There was a problem hiding this comment.
Nit: ${attributes} property not an object
* Wrap 'no-fill' attribute under AdsenseNetworkConfig. * Wrap no-fill attribute under AdsenseNetworkConfig * resolve violation errors. * Adding sticky ad attributes from config object. * Adding enum for attributes of different ad formats. * updating getAttributesFromConfigObj from placement.js. * Updating amp auto ads MD file.
