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

Demo update #141

Merged
merged 1 commit into from
Sep 18, 2015
Merged

Demo update #141

merged 1 commit into from
Sep 18, 2015

Conversation

erwinmombay
Copy link
Member

<html>
<head>
<meta charset="utf-8">
<title>AMP #0</title>
Copy link
Member

Choose a reason for hiding this comment

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

Some other title please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@cramforce
Copy link
Member

Please check the license requirements for the used asset and text.

What type of license does this fall under https://commons.wikimedia.org/wiki/Commons:Reusing_content_outside_Wikimedia

<main role="main">
<article>
<figure>
<amp-img src="img/Mountfujijapan.jpg" layout="responsive" width="360" placeholder
Copy link
Member

Choose a reason for hiding this comment

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

Did you see that we automatically create examples.build that has generated files pointing to the minified version and prod?

You probably need to change the img references to /examples/img
for those to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, i'll update.

Copy link
Member Author

Choose a reason for hiding this comment

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

done. and added to gulpfile:

examplesWithMinifiedJs('demo.amp.html');

@cramforce
Copy link
Member

LGTM

Lets hand this and back out before launch based on feedback from OSS.

@erwinmombay erwinmombay self-assigned this Sep 16, 2015
@cramforce
Copy link
Member

Did you see the email? Maybe put the images into a dedicated dir with a LICENSE file pointing to where you got this from.

@erwinmombay
Copy link
Member Author

@cramforce yep i did, will do that. thanks for the suggestion.

@erwinmombay
Copy link
Member Author

@cramforce how about the copy? should i move the demo.amp.html into third party? (if so we'll have to tweak the build so it gets picked up, etc)

@erwinmombay
Copy link
Member Author

@cramforce so I'm told since the copy is 100 years old that is ok to use, but since the images are public domain, it means there is no license and we can't use them. I'll go ahead and just use the one image you took a few days ago and add another one that I'll take later if that is ok with you.

@erwinmombay
Copy link
Member Author

@cramforce PTAL. I'll work with Ed further to improve this as well as make new ones (especially the assets part).

screen shot 2015-09-17 at 11 46 01 am

@@ -0,0 +1,6 @@
!_TAG_FILE_FORMAT 2 /extended format/
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. was running ctags (jstags) accidentally committed it.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed.

@cramforce
Copy link
Member

Lets have some pretty pictures. The text can just be lorem ipsum.
I'm putting these images I took under the Apache 2 license of this project https://goo.gl/photos/hDh7vzah1GVo2e4r8

@erwinmombay
Copy link
Member Author

@cramforce great, i've switched it to lorem ipsum. let me know if the images are in.

@cramforce
Copy link
Member

@erwinmombay You can just download them from the link. And maybe resize as you need them

@erwinmombay
Copy link
Member Author

@cramforce PTAL

screen shot 2015-09-17 at 3 16 18 pm

@erwinmombay
Copy link
Member Author

fixed presubmit

@erwinmombay
Copy link
Member Author

hah, have to submit the fix for the presubmit in another PR or else this will never pass lol

<article>
<figure>
<amp-img
src="/examples/img/IMG_20150904_181705@1x.jpg"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe give this a semantic name?

Copy link
Member Author

Choose a reason for hiding this comment

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

done. renamed to hero.jpg and sea.jpg

@cramforce
Copy link
Member

LGTM

@cramforce
Copy link
Member

Please update the demo accordingly to the new rules (async, plus extra style tags)

@erwinmombay
Copy link
Member Author

will do

@erwinmombay
Copy link
Member Author

added new style and noscript tags to doc.
added async prop to amp file.

erwinmombay added a commit that referenced this pull request Sep 18, 2015
@erwinmombay erwinmombay merged commit ebfd2a7 into ampproject:master Sep 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants