Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add amp-sticky-ad #59

Open
2 tasks
pradeep910 opened this issue Oct 11, 2019 · 17 comments · May be fixed by #62
Open
2 tasks

Add amp-sticky-ad #59

pradeep910 opened this issue Oct 11, 2019 · 17 comments · May be fixed by #62
Assignees
Labels
enhancement New feature or request

Comments

@pradeep910
Copy link
Contributor

pradeep910 commented Oct 11, 2019

Add support for amp sticky ad for mobile - https://amp.dev/documentation/components/amp-sticky-ad/

We can add an option in the plugin options page -

  • Enable/Disable Sticky Ad
  • Adunit name for sticky ad (for example - /35096353/amptesting/formats/sticky)

Enabling this will add amp-sticky-ad that shows at the footer of page.
Out put this markup with dynamic values from options page -

<amp-sticky-ad layout="nodisplay">
  <amp-ad
    width="320"
    height="50"
    type="doubleclick"
    data-slot="/35096353/amptesting/formats/sticky"
  >
  </amp-ad>
</amp-sticky-ad>
@deepaklalwani97
Copy link
Member

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

@pradeep910
Copy link
Contributor Author

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

@pradeep910 pradeep910 added the enhancement New feature or request label Jan 23, 2020
@pradeep910 pradeep910 added this to the AMP Admanager 1.0 milestone Jan 23, 2020
@deepaklalwani97
Copy link
Member

deepaklalwani97 commented Jan 23, 2020

@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.

@deepaklalwani97 deepaklalwani97 linked a pull request Jan 23, 2020 that will close this issue
@pradeep910
Copy link
Contributor Author

@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

@deepaklalwani97
Copy link
Member

@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.

@deepaklalwani97
Copy link
Member

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.

@sagarnasit
Copy link
Member

sagarnasit commented Jan 24, 2020

@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 => '.....'
] );

@pradeep910
Copy link
Contributor Author

@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 => '.....'
] );

@deepaklalwani97
Copy link
Member

deepaklalwani97 commented Jan 27, 2020

@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.

@deepaklalwani97
Copy link
Member

deepaklalwani97 commented Jan 27, 2020

@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?

@sagarnasit
Copy link
Member

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.

@deepaklalwani97
Copy link
Member

deepaklalwani97 commented Jan 28, 2020

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

@sagarnasit
Copy link
Member

@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.

@deepaklalwani97
Copy link
Member

@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.

@deepaklalwani97
Copy link
Member

@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?

@pradeep910
Copy link
Contributor Author

@deepaklalwani97 Lets use fixed size for sticky ad.

@deepaklalwani97
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants