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

Adds new "call to action" layer #13631

Merged
merged 4 commits into from Feb 26, 2018
Merged

Adds new "call to action" layer #13631

merged 4 commits into from Feb 26, 2018

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Feb 23, 2018


#12169

This PR includes corresponding styles, and an example of it interacting with the existing amp-story-grid-layer templates.

A subsequent PR will be sent to include validation logic.

Example:

image

…mple of it interacting with the existing amp-story-grid-layer templates.

<meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
<link rel="canonical" href="grid-layer-templates.html">
<style amp-boilerplate>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably make this one line. Try copying and pasting the whole doc into https://validator.ampproject.org and make sure it's valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Now it only complains about the new element since it's not supported by the validator yet. (Will be done in next PR).

@@ -0,0 +1,53 @@
/**
* Copyright 2017 The AMP HTML Authors. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It's 2018 now 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like to live in the past.

}
}

AMP.registerElement('amp-story-cta-layer', AmpStoryCtaLayer);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a newline at the end of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

z-index: 3 !important;
}

amp-story-cta-layer * {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably drop this whole style; it's applied to grid layer for legacy reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

</amp-story>
</body>

</html>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: new line at the end of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

</amp-story-cta-layer>
</amp-story-page>


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: here and at the top of the file, maybe only one line break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@lannka
Copy link
Contributor

lannka commented Feb 23, 2018

/cc @calebcordry

@calebcordry
Copy link
Member

Cool! amp-story-auto-ads will be creating these in upcoming PR

@newmuis newmuis merged commit dfb6ea1 into ampproject:master Feb 26, 2018
RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
* Adds new "call to action" layer with corresponding styles, and an example of it interacting with the existing amp-story-grid-layer templates.

* Removes unecessary tyling for child elements, fixes example to comply with validator, fixes formatting.

* Fixes formatting.

* fixup! Fixes formatting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants