Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

Add support for sticky ad#62

Open
deepaklalwani97 wants to merge 16 commits intodevelopfrom
feature/add-sticky-ad-support
Open

Add support for sticky ad#62
deepaklalwani97 wants to merge 16 commits intodevelopfrom
feature/add-sticky-ad-support

Conversation

@deepaklalwani97
Copy link
Copy Markdown
Member

@deepaklalwani97 deepaklalwani97 commented Jan 23, 2020

Fixes: #59

rtBot
rtBot suggested changes Jan 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpcs scanning turned up:

🚫 5 errors

hashes-api-scanning skipped

Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-admin.php
Comment thread classes/class-admin.php
rtBot
rtBot suggested changes Jan 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpcs scanning turned up:

🚫 1 error

hashes-api-scanning skipped

Comment thread classes/class-amp-admanager.php Outdated
rtBot
rtBot suggested changes Jan 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpcs scanning turned up:

🚫 1 error

hashes-api-scanning skipped

Comment thread classes/class-amp-admanager.php Outdated
@deepaklalwani97 deepaklalwani97 changed the title WIP: Add support for sticky ad Add support for sticky ad Jan 24, 2020
@deepaklalwani97 deepaklalwani97 mentioned this pull request Jan 24, 2020
Open
2 tasks
sagarnasit
sagarnasit reviewed Jan 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@sagarnasit sagarnasit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added minor feedback.

Comment thread classes/class-amp-admanager.php
Comment thread classes/class-amp-admanager.php Outdated
Comment on lines +475 to +477
'desktop-sizes' => '600x90,728x90',
'tablet-sizes' => '320x50,468x60',
'mobile-sizes' => '320x50,468x60',
Copy link
Copy Markdown
Member

@sagarnasit sagarnasit Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizes should be dynamic instead of static, right? I see the discussion is going on #59
cc: @pradeep910

Copy link
Copy Markdown
Contributor

@pradeep910 pradeep910 Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sizes should be dynamic, and override with these *-sizes parameters.

rtBot
rtBot suggested changes Jan 28, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpcs scanning turned up:

🚫 1 error

hashes-api-scanning skipped

Comment thread classes/class-amp-admanager.php
pradeep910
pradeep910 reviewed Jan 30, 2020
View reviewed changes
Comment thread template-parts/amp-resources.php Outdated

<?php //phpcs:disable ?>
<script type="text/javascript" src="https://cdn.ampproject.org/v0.js" async></script>
<script async custom-element="amp-sticky-ad" src="https://cdn.ampproject.org/v0/amp-sticky-ad-1.0.js"></script>
Copy link
Copy Markdown
Contributor

@pradeep910 pradeep910 Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a check here if sticky ad is enabled, then only include this script.

pradeep910
pradeep910 suggested changes Jan 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@pradeep910 pradeep910 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small feedback.

rtBot
rtBot suggested changes Jan 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpcs scanning turned up:

🚫 1 error

hashes-api-scanning skipped

Comment thread template-parts/amp-resources.php
@deepaklalwani97
Copy link
Copy Markdown
Member Author

deepaklalwani97 commented Jan 30, 2020

Addressed feedbacks

@deepaklalwani97 deepaklalwani97 changed the base branch from master to develop February 7, 2020 13:20
@deepaklalwani97 deepaklalwani97 changed the title Add support for sticky ad WIP: Add support for sticky ad Apr 3, 2020
rtBot
rtBot suggested changes Apr 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpcs scanning turned up:

🚫 6 errors

hashes-api-scanning skipped

Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-amp-admanager.php Outdated
Comment thread classes/class-amp-admanager.php Outdated
@deepaklalwani97 deepaklalwani97 changed the title WIP: Add support for sticky ad Add support for sticky ad Jun 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pradeep910 pradeep910 Awaiting requested review from pradeep910

2 more reviewers

@sagarnasit sagarnasit sagarnasit left review comments

@rtBot rtBot rtBot requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add amp-sticky-ad

4 participants

@deepaklalwani97 @pradeep910 @sagarnasit @rtBot