Amp flying carpet #169

kul3r4 · 2016-05-27T16:23:10Z

An example for #3126 and #159.
@juliantoledo PTAL

juliantoledo · 2016-05-30T12:50:40Z

src/20_Components/amp-flying-carpet.html

+      padding-bottom: 8px;
+    }
+
+  .parent {


small nit, indentation.

juliantoledo · 2016-05-30T13:21:00Z

@kul3r4
Looks good, I just added few comments about indentation and the lorem ipsum text in the first column. I also think that maybe the file should also be amp-fx-flying-carpet to match the filename and page title to the component name.

sebastianbenz · 2016-06-01T06:34:15Z

src/20_Components/amp-flying-carpet.html

+      padding-top: 8px;
+      padding-bottom: 8px;
+    }
+


do we need ad-separator and ad-label? Also, do we need parent?

sebastianbenz · 2016-06-01T06:43:22Z

Nice! Few comments:

why do we need the additional text and image?
we should enable the preview mode
can we add a link the official doc (we should do this for all samples).

kul3r4 · 2016-06-01T08:57:11Z

@sebastianbenz Additional images and content are needed because amp-flying-carpet cannot appear in the first or last viewport, so I am using text and images to fill the page.
I'll fix the other comments.

sebastianbenz · 2016-06-01T08:58:55Z

We should mention this in the doc!

kul3r4 · 2016-06-01T09:02:44Z

It's mentioned in the example:
"Positioning

amp-fx-flying-carpet elements must be positioned before the last viewport so make sure you add some content after the component.

The text that follows is just for making the page scrollable to visualize the image inside the last amp-fx-flying-carpet."

sebastianbenz · 2016-06-01T09:21:05Z

Sorry - my fault. As you can see - people don't read the text at the bottom if there is no matching example.

Suggestion:

move the "Positioning" section to the first Lorem Ipsum sections.
add another doc section at the bottom Lorem Ipsum.
Move the bottom Lorem Ipsum to the preview section

kul3r4 · 2016-06-01T17:08:16Z

PTAL.
Please wait for merging when it will look good for you.

juliantoledo · 2016-06-02T08:51:13Z

We are getting a 404 for https://cdn.ampproject.org/v0/amp-fx-flying-carpet-0.1.js at the moment. We will hold merging this until amp-fx-flying-carpet is in prod later today.
(context ampproject/amphtml#3336)

kul3r4 · 2016-06-06T08:45:35Z

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

sebastianbenz · 2016-06-06T08:49:34Z

src/20_Components/amp-fx-flying-carpet.html

+
+  <!--
+    `amp-fx-flying-carpet` can be used to display images.
+    Use height parameter to specify the height of the flying carpets "window".


s/height parameter/height parameter/

sebastianbenz · 2016-06-06T09:12:02Z

Added some comments.

Does it make sense to enable preview mode or wouldn't this work because the flying carpet would be in the first viewport?

kul3r4 · 2016-06-06T10:15:26Z

I didn't enable the preview because this would require even more text to make sure the flying carpet be after the first viewport.(at least when viewing that from a monitor and not from mobile)

sebastianbenz · 2016-06-06T16:04:55Z

LGTM

jasti · 2016-06-07T22:19:24Z

src/20_Components/amp-fx-flying-carpet.html

+  -->
+  <!--
+
+    `amp-fx-flying-carpet` elements must be positioned after the first viewport


Also can't be placed in the last viewport.

Thanks, there is already a comment like that if you scroll the page, it says:

`amp-fx-flying-carpet` elements must be positioned before the last viewport

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

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 reviewed May 30, 2016
View reviewed changes

src/20_Components/amp-flying-carpet.html

padding-bottom: 8px;

}

.parent {

Copy link

Collaborator

juliantoledo May 30, 2016

Choose a reason for hiding this comment

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

small nit, indentation.

juliantoledo added this to the Samples for May/June milestone May 31, 2016

sebastianbenz reviewed Jun 1, 2016
View reviewed changes

kul3r4 force-pushed the amp-flying-carpet branch 3 times, most recently from bcd17b1 to 6ce2097 Compare June 1, 2016 17:06

kul3r4 force-pushed the amp-flying-carpet branch 4 times, most recently from 14a11fe to 88cce59 Compare June 3, 2016 08:36

sebastianbenz reviewed Jun 6, 2016
View reviewed changes

kul3r4 force-pushed the amp-flying-carpet branch from 88cce59 to a935dd2 Compare June 6, 2016 10:06

Added example

e9bc45e

kul3r4 force-pushed the amp-flying-carpet branch from a935dd2 to e9bc45e Compare June 6, 2016 14:33

sebastianbenz merged commit f772ab7 into ampproject:master Jun 6, 2016

jasti reviewed Jun 7, 2016
View reviewed changes

kul3r4 added a commit that referenced this pull request Jun 10, 2016

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

2d05510

kul3r4 added a commit that referenced this pull request Jun 10, 2016

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

b61f150

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amp flying carpet #169

Amp flying carpet #169

kul3r4 commented May 27, 2016 •

edited

juliantoledo May 30, 2016

juliantoledo commented May 30, 2016 •

edited

sebastianbenz Jun 1, 2016 •

edited

sebastianbenz commented Jun 1, 2016

kul3r4 commented Jun 1, 2016

sebastianbenz commented Jun 1, 2016

kul3r4 commented Jun 1, 2016

sebastianbenz commented Jun 1, 2016

kul3r4 commented Jun 1, 2016

juliantoledo commented Jun 2, 2016 •

edited

kul3r4 commented Jun 6, 2016

sebastianbenz Jun 6, 2016 •

edited

kul3r4 Jun 6, 2016

sebastianbenz commented Jun 6, 2016 •

edited

kul3r4 commented Jun 6, 2016

sebastianbenz commented Jun 6, 2016

jasti Jun 7, 2016

kul3r4 Jun 8, 2016

Amp flying carpet #169

Amp flying carpet #169

Conversation

kul3r4 commented May 27, 2016 • edited

juliantoledo May 30, 2016

Choose a reason for hiding this comment

juliantoledo commented May 30, 2016 • edited

sebastianbenz Jun 1, 2016 • edited

Choose a reason for hiding this comment

sebastianbenz commented Jun 1, 2016

kul3r4 commented Jun 1, 2016

sebastianbenz commented Jun 1, 2016

kul3r4 commented Jun 1, 2016

sebastianbenz commented Jun 1, 2016

kul3r4 commented Jun 1, 2016

juliantoledo commented Jun 2, 2016 • edited

kul3r4 commented Jun 6, 2016

sebastianbenz Jun 6, 2016 • edited

Choose a reason for hiding this comment

kul3r4 Jun 6, 2016

Choose a reason for hiding this comment

sebastianbenz commented Jun 6, 2016 • edited

kul3r4 commented Jun 6, 2016

sebastianbenz commented Jun 6, 2016

jasti Jun 7, 2016

Choose a reason for hiding this comment

kul3r4 Jun 8, 2016

Choose a reason for hiding this comment

kul3r4 commented May 27, 2016 •

edited

juliantoledo commented May 30, 2016 •

edited

sebastianbenz Jun 1, 2016 •

edited

juliantoledo commented Jun 2, 2016 •

edited

sebastianbenz Jun 6, 2016 •

edited

sebastianbenz commented Jun 6, 2016 •

edited