Skip to content
This repository has been archived by the owner on Oct 22, 2019. It is now read-only.

Added sticky ad to amp-ad example #165

Merged
merged 1 commit into from Jun 10, 2016
Merged

Added sticky ad to amp-ad example #165

merged 1 commit into from Jun 10, 2016

Conversation

kul3r4
Copy link
Collaborator

@kul3r4 kul3r4 commented May 17, 2016

@kul3r4
Copy link
Collaborator Author

kul3r4 commented May 17, 2016

Validator is not supporting amp-sticky-ad, so I created this

@kul3r4
Copy link
Collaborator Author

kul3r4 commented May 25, 2016

@jmadler PTAL

@@ -53,6 +57,19 @@
</div>
</amp-ad>

<!-- #### Sticky Ads -->
<!--
`amp-sticky-ad` adds supports for ad units that always take a fixed place in the viewport.
Copy link
Contributor

Choose a reason for hiding this comment

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

These units are often called "adhesion" units, so I'd add that word for folks who are more familiar with it

Copy link
Member

Choose a reason for hiding this comment

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

"adhesion unit" is something very specific (at least to some people) that is different in specifics from amp-sticky-ad. Should this be in our examples before we tried it with a few pubs?

@kul3r4 kul3r4 force-pushed the sticky-ads branch 4 times, most recently from 7807c52 to 4b8a440 Compare May 27, 2016 18:30
@kul3r4
Copy link
Collaborator Author

kul3r4 commented May 27, 2016

I changed the add that i was using for the sticky one so that is smaller than 100px and fits the container.
@cramforce did you meant you want to wait to publish an example with amp-sticky-ad?

@cramforce
Copy link
Member

@kul3r4 Yeah, I thought, we might test this out with a few publishers before giving it publicity. On the other hand, we did publish the docs, so we might as well :)

@kashyapnitin What you do you think?

@kul3r4
Copy link
Collaborator Author

kul3r4 commented Jun 6, 2016

@sebastianbenz @juliantoledo PTAL, we are trying to get this ready today.

@sebastianbenz
Copy link
Collaborator

amp-sticky-add is its own component. I think this would justify putting it in its own sample file. Thoughts?

@kul3r4 kul3r4 force-pushed the sticky-ads branch 2 times, most recently from acc276a to 5e7c710 Compare June 6, 2016 11:42
@kul3r4
Copy link
Collaborator Author

kul3r4 commented Jun 6, 2016

I created a separate example, PTAL.

@kul3r4
Copy link
Collaborator Author

kul3r4 commented Jun 6, 2016

Using a different ad to get the experiment more robust.

@jasti
Copy link

jasti commented Jun 7, 2016

@kul3r4 There are two optional params that can be passed to the sticky ad declaration for the background of the container and background of the container while load time. @zhouyx is working on updating the README, but once we have that, would be great to have those params showcased in the example.

@zhouyx
Copy link
Contributor

zhouyx commented Jun 7, 2016

@jasti Am I understand the requirement wrong? Now the two background setting params are not passed to sticky ad declaration. I have them set through standard CSS.
ampproject/amphtml#3486

@jasti
Copy link

jasti commented Jun 7, 2016

Thanks @zhouyx . I think it's fine to expose these using standard CSS classes.
@kul3r4 Would be great to have an extra example with modified (using CSS) background setting params on the page.

@kul3r4
Copy link
Collaborator Author

kul3r4 commented Jun 8, 2016

@zhouyx @jasti , I tried to add:

.amp-sticky-ad { background-color: #fce4ec } .amp-sticky-ad-loaded { background-color: #f48fb1 }

in the <style amp-custom> section, but when the page is rendered, I don't see those two classes being added to the html, so the background does not get set.

I don't see those classes being added into the code, am I missing something?

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot
Copy link

CLAs look good, thanks!

@kul3r4
Copy link
Collaborator Author

kul3r4 commented Jun 10, 2016

@sebastianbenz PTAL

@sebastianbenz
Copy link
Collaborator

LGTM

@sebastianbenz sebastianbenz merged commit d205fc8 into master Jun 10, 2016
@sebastianbenz sebastianbenz deleted the sticky-ads branch June 10, 2016 14:39
@jasti
Copy link

jasti commented Jun 10, 2016

@zhouyx any thoughts on @kul3r4 's question on adjusting background colors please?

.amp-sticky-ad { background-color: #fce4ec } .amp-sticky-ad-loaded { background-color: #f48fb1 }
in the <style amp-custom> section, but when the page is rendered, I don't see those two classes being added to the html, so the background does not get set.
I don't see those classes being added into the code, am I missing something?

@zhouyx
Copy link
Contributor

zhouyx commented Jun 10, 2016

@kul3r4 The code should be in canary today. And those two classes should be available then.

juliantoledo added a commit that referenced this pull request Jun 13, 2016
Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

add working example of amp-install-serviceworker (#156)

* add amp-install-serviceworker sample

* add comments on sw-precache

* add comment on how the SW works in this demo and how to confirm the result

* change to cache all image, video, html

* fix to point to original ABE page

* fix lint of gulpfile.js

* fix to cache only amp-install-serviceworker related files

* add comments on benefits of SW and changed only to cache amp-install-serviceworker

* nit

* modify comments and changed path to sw.js in relative path

* change gulpfile to organize the process to generate sw.js

* change gulpfile.js to directory put sw.js in dist dir

improve amp-social-share sample

* add amp facebook sample
* remove experimental flag
* add samples for all providers

Add amp-fx-flying-carpet sample (#169)

Change cursor to pointer on mouseover (#179)

Improve amp-accordion sample (#184)

* demonstrate better show/hide
* show simple with single show button

Fixes #182,#183.

Added ad to amp-fx-flying-carpet (#186)

optimize images

add AMP URL API embed

add AMP URL API Sample

Added sticky-ad example (#165)

Use a new ad (#187)

Remove the non-working link to create a fork

Fix typo in NewsArticle sample

disable directory file listings

Fixes #82

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button
juliantoledo added a commit that referenced this pull request Jun 14, 2016
Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

add working example of amp-install-serviceworker (#156)

* add amp-install-serviceworker sample

* add comments on sw-precache

* add comment on how the SW works in this demo and how to confirm the result

* change to cache all image, video, html

* fix to point to original ABE page

* fix lint of gulpfile.js

* fix to cache only amp-install-serviceworker related files

* add comments on benefits of SW and changed only to cache amp-install-serviceworker

* nit

* modify comments and changed path to sw.js in relative path

* change gulpfile to organize the process to generate sw.js

* change gulpfile.js to directory put sw.js in dist dir

improve amp-social-share sample

* add amp facebook sample
* remove experimental flag
* add samples for all providers

Add amp-fx-flying-carpet sample (#169)

Change cursor to pointer on mouseover (#179)

Improve amp-accordion sample (#184)

* demonstrate better show/hide
* show simple with single show button

Fixes #182,#183.

Added ad to amp-fx-flying-carpet (#186)

optimize images

add AMP URL API embed

add AMP URL API Sample

Added sticky-ad example (#165)

Use a new ad (#187)

Remove the non-working link to create a fork

Fix typo in NewsArticle sample

disable directory file listings

Fixes #82

Remove AMP URL API Wrapper

* no longer needed as API now supports CORS
* rename view folder -> iframe
* increase result view size to match default result

add separator between amp-fx-flying-carpet and the text, add background and use another ad (#195)

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button
juliantoledo added a commit that referenced this pull request Jun 14, 2016
Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

add working example of amp-install-serviceworker (#156)

* add amp-install-serviceworker sample

* add comments on sw-precache

* add comment on how the SW works in this demo and how to confirm the result

* change to cache all image, video, html

* fix to point to original ABE page

* fix lint of gulpfile.js

* fix to cache only amp-install-serviceworker related files

* add comments on benefits of SW and changed only to cache amp-install-serviceworker

* nit

* modify comments and changed path to sw.js in relative path

* change gulpfile to organize the process to generate sw.js

* change gulpfile.js to directory put sw.js in dist dir

improve amp-social-share sample

* add amp facebook sample
* remove experimental flag
* add samples for all providers

Add amp-fx-flying-carpet sample (#169)

Change cursor to pointer on mouseover (#179)

Improve amp-accordion sample (#184)

* demonstrate better show/hide
* show simple with single show button

Fixes #182,#183.

Added ad to amp-fx-flying-carpet (#186)

optimize images

add AMP URL API embed

add AMP URL API Sample

Added sticky-ad example (#165)

Use a new ad (#187)

Remove the non-working link to create a fork

Fix typo in NewsArticle sample

disable directory file listings

Fixes #82

Remove AMP URL API Wrapper

* no longer needed as API now supports CORS
* rename view folder -> iframe
* increase result view size to match default result

add separator between amp-fx-flying-carpet and the text, add background and use another ad (#195)

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button
juliantoledo added a commit that referenced this pull request Jun 14, 2016
Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

add working example of amp-install-serviceworker (#156)

* add amp-install-serviceworker sample

* add comments on sw-precache

* add comment on how the SW works in this demo and how to confirm the result

* change to cache all image, video, html

* fix to point to original ABE page

* fix lint of gulpfile.js

* fix to cache only amp-install-serviceworker related files

* add comments on benefits of SW and changed only to cache amp-install-serviceworker

* nit

* modify comments and changed path to sw.js in relative path

* change gulpfile to organize the process to generate sw.js

* change gulpfile.js to directory put sw.js in dist dir

improve amp-social-share sample

* add amp facebook sample
* remove experimental flag
* add samples for all providers

Add amp-fx-flying-carpet sample (#169)

Change cursor to pointer on mouseover (#179)

Improve amp-accordion sample (#184)

* demonstrate better show/hide
* show simple with single show button

Fixes #182,#183.

Added ad to amp-fx-flying-carpet (#186)

optimize images

add AMP URL API embed

add AMP URL API Sample

Added sticky-ad example (#165)

Use a new ad (#187)

Remove the non-working link to create a fork

Fix typo in NewsArticle sample

disable directory file listings

Fixes #82

Remove AMP URL API Wrapper

* no longer needed as API now supports CORS
* rename view folder -> iframe
* increase result view size to match default result

add separator between amp-fx-flying-carpet and the text, add background and use another ad (#195)

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button
juliantoledo added a commit that referenced this pull request Jun 14, 2016
Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

add working example of amp-install-serviceworker (#156)

* add amp-install-serviceworker sample

* add comments on sw-precache

* add comment on how the SW works in this demo and how to confirm the result

* change to cache all image, video, html

* fix to point to original ABE page

* fix lint of gulpfile.js

* fix to cache only amp-install-serviceworker related files

* add comments on benefits of SW and changed only to cache amp-install-serviceworker

* nit

* modify comments and changed path to sw.js in relative path

* change gulpfile to organize the process to generate sw.js

* change gulpfile.js to directory put sw.js in dist dir

improve amp-social-share sample

* add amp facebook sample
* remove experimental flag
* add samples for all providers

Add amp-fx-flying-carpet sample (#169)

Change cursor to pointer on mouseover (#179)

Improve amp-accordion sample (#184)

* demonstrate better show/hide
* show simple with single show button

Fixes #182,#183.

Added ad to amp-fx-flying-carpet (#186)

optimize images

add AMP URL API embed

add AMP URL API Sample

Added sticky-ad example (#165)

Use a new ad (#187)

Remove the non-working link to create a fork

Fix typo in NewsArticle sample

disable directory file listings

Fixes #82

Remove AMP URL API Wrapper

* no longer needed as API now supports CORS
* rename view folder -> iframe
* increase result view size to match default result

add separator between amp-fx-flying-carpet and the text, add background and use another ad (#195)

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button

New drawer menu using amp-sidebar

Cherry picked 3aed625
New header design:
* collapse header on mobile devices
* add AMP logo
* introduce hamburger button
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants