Add amp-sticky-ad about amp-admanager HOT 17 OPEN

@pradeep910 we should give an attribute option to user for sticky ad where he can specify other attributes as well.

example

AMP_AdManger::get_ads( [
    sticky => true,
    mobile_sizes   => '.....',
    tablet_sizes     => '.....'
    desktop_sizes => '.....'
] );

or
Use separate function for sticky ad when user just have to pass other attributes.

AMP_AdManger::get_sticky_ad( [
    mobile_sizes   => '.....',
    tablet_sizes     => '.....'
    desktop_sizes => '.....'
] );

from amp-admanager.

@deepaklalwani97 Settings remain same. Please do not add sticky ad in footer, instead user will config sticky ad like normal ad by passing required attributes. What is different for sticky ad is that we will use separate function get_amp_sticky_ad. This function will call get_amp_ad internally in which we will pass dynamic attributes passed by developer, in addition to that we will pass sticky ad required attributes with them. For end developer configs remain same but have to call different function for different ad type.

from amp-admanager.

@pradeep910 Can you share more details on what needs to be done in this issue.

from amp-admanager.

@deepaklalwani97 Added description pls let me know if you still have questions.

from amp-admanager.

@pradeep910 Should I add fields for height and width to the options page or are we going to use a fixed height and width for the sticky ad. We cannot use multiple breakpoints for the sticky ad as only one sticky ad is allowed per page otherwise it will not show sticky if there are multiple.

from amp-admanager.

@deepaklalwani97 Width and height can be passed through function, but ideally it only should show banner / horizontal ads.
The function will set width and height using this sizes attribute or whatever we give in breakpoints like desktop-sizes, tablet-sizes, etc.

$ad_attr = [
						'ad-unit'       => 'GK_Header',
						'desktop-sizes' => '600x90,728x90',
						'tablet-sizes'  => '320x50,468x60',
						'mobile-sizes'  => '320x50,468x60',
						'sizes' => '320x50,728x90',
						'layout'        => 'fixed',
					];

Default should be 320x50

from amp-admanager.

@pradeep910 Yes I know my question is we want to pass these sizes as hardcoded in function or provide a setting for the user to add.

from amp-admanager.

I have added different sizes according to the devices. Let me know if we want a setting for that. This PR is ready for review #62.

from amp-admanager.

@sagarnasit @deepaklalwani97
I think separate function will be good idea.
Please go ahead with this -

AMP_AdManger::get_sticky_ad( [
    mobile_sizes   => '.....',
    tablet_sizes     => '.....'
    desktop_sizes => '.....'
] );

from amp-admanager.

@pradeep910 @sagarnasit I don't think the separate function would make any difference it would just create the redundancy as the code in both the functions would almost be the same. Still, let me know your thoughts and if we want to go ahead with this.

from amp-admanager.

@pradeep910 About the size of ads do we want to keep a specific static size for all devices or dynamically passed by the user from settings?

from amp-admanager.

I don't think the separate function would make any difference it would just create the redundancy as the code in both the functions would almost be the same.

@deepaklalwani97 Whole purpose of making a separate function is to make it developer friendly and easy to understand the functionality. Under the hood it will call the get_ads function with required sticky ads attributes so there should not be any redundancy.

About the size of ads do we want to keep a specific static size for all devices or dynamically passed by the user from settings?

We will also need to allow dynamic passing of attributes like we are doing it for normal amp ad.

from amp-admanager.

@sagarnasit Should I add three settings for different device sizes( i.e mobile, tablet, desktop ) for a user to add?

from amp-admanager.

@sagarnasit @pradeep910 The suggested changes are addressed in #62. Can you please re-review again and approve. so I can get this tested by QA and merge it.

from amp-admanager.

@pradeep910 We cannot have multiple breakpoints for the amp sticky ad. Because it will throw validation error in reader mode and remove other amp-sticky-ad markup in transitional and standard mode if there are multiple sticky ads on a page and we can use only one amp-ad inside the amp-sticky-ad. Can we have one fixed-size sticky amp ad or let the user select the size for the sticky ad from the settings?

from amp-admanager.

@deepaklalwani97 Lets use fixed size for sticky ad.

from amp-admanager.

I have updated the PR to add setting fields for height and width to be applied to the sticky ad.

from amp-admanager.