Skip to content

Subscriptions: fixing examples#15129

Merged
prateekbh merged 6 commits intoampproject:masterfrom
prateekbh:fix-examples
May 8, 2018
Merged

Subscriptions: fixing examples#15129
prateekbh merged 6 commits intoampproject:masterfrom
prateekbh:fix-examples

Conversation

@prateekbh
Copy link
Copy Markdown
Member

  • Fix amp example
  • Whitelist subscription attributes
  • Execute action from amp-subscriptions-google

@prateekbh prateekbh requested a review from dvoytenko May 7, 2018 17:39
subscriptions-action="login"
subscriptions-service="subscribe.google.com"
subscriptions-decorate>Login</div>
<div subscriptions-action="login" subscriptions-display="NOT loggedIn">Login</div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NOT loggedIn is now wrong too, right? Shouldn't it be NOT data.loggedIn?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep

</template>
<template type="amp-mustache" subscriptions-dialog subscriptions-display="NOT loggedIn">
<div>Dialog for non logged in users<div>
<div subscriptions-display="NOT loggedIn"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

'subscriptions-section',
'subscriptions-display',
'subscriptions-service',
'subscriptions-decorate',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you already put subscrpitions-decorate into validator?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done now

Copy link
Copy Markdown
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

LGTM with one request

subscriptions-action="subscribe"
subscriptions-service="subscribe.google.com"
subscriptions-decorate>Login</div>
<div subscriptions-display="NOT grantReason">btw u are not subscribed</div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's do grantReason != "SUBSCRIBED" (SUBSCRIBER, whichever is the latest on this.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

reverted this and loggedIn change as that stuff is in PR #15059

@prateekbh prateekbh merged commit ff88a58 into ampproject:master May 8, 2018
@prateekbh prateekbh deleted the fix-examples branch May 8, 2018 01:08
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request May 10, 2018
* fixing examples

* fixing types

* fixing types

* adding in validatior

* reverting data changes in example
honeybadgerdontcare added a commit that referenced this pull request May 10, 2018
* Revision bump for #14969

* Revision bump for #14937

* Simplify the script tag error.

* Whitelist pro.fontawesome.com in the AMP font list.

* Error category remapping

* Revision bump for #15106

* Revision bump for #15129

* Revision bump for #15155/15164/15191/15187
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.

3 participants