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

📖 [AMP Story Paywall] amp-story-subscriptions documentation #38179

Merged
merged 11 commits into from
Jul 18, 2022

Conversation

ychsieh
Copy link
Contributor

@ychsieh ychsieh commented May 10, 2022

The public markdown file in this PR is based on the internal API documentation draft here: go/amp-story-subscriptions-documentation

@amp-owners-bot
Copy link

amp-owners-bot bot commented May 10, 2022

Hey @newmuis, @gmajoulet! These files were changed:

extensions/amp-story-subscriptions/0.1/amp-story-subscriptions.md

@ychsieh ychsieh requested a review from calebcordry May 10, 2022 18:21
@ychsieh ychsieh removed the request for review from calebcordry May 10, 2022 18:26
@newmuis
Copy link
Contributor

newmuis commented May 11, 2022

Is there a reason for this to be internal? Can you convert it to markdown and include it here?

@ychsieh
Copy link
Contributor Author

ychsieh commented May 11, 2022

The public markdown file is in fact included in this PR and is based on the provided doc link in the PR description. Updated the PR description to make it clear.

@ychsieh
Copy link
Contributor Author

ychsieh commented May 13, 2022

Friendly ping.

Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

Approving with a nit.
I'll leave it to @CrystalOnScript for the final review!

Copy link
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

Overall looks good, but requesting some clarification on the attributes

...
```

See details about specifying this configuration in [`amp-subscriptions`](https://amp.dev/documentation/components/amp-subscriptions/).
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no difference between the configuration for a Story or classic AMP, I think we could call it out specifically (you can port your existing configuration...) and just link to the doc without giving an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Removed this section and call it out in the summary section.

</amp-story>
```

### `publisher-logo-src` {string} required
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get it from the amp-story metadata? https://github.com/ampproject/amphtml/blob/main/extensions/amp-story/amp-story.md#publisher-logo-src-guidelines
Do publishers need to repeat it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is from amp-story's attribute. I was trying to express that this amp-story attribute is mandatory for this extension to work. But yes, this attribute is already mandatory. We don't need to repeatedly saying that here. Removed this entirely.


A URL to the story publisher's logo in square format (1x1 aspect ratio). This logo would be shown on the publisher's subscribe button. For example `publisher-logo-src="https://example.com/logo/1x1.png"`, where 1x1.png is a 96x96 px logo. See the guidelines at [`amp-story` dev document](https://amp.dev/documentation/components/amp-story/?format=stories#publisher-logo-src-guidelines).

### `subscriptions-page-index` {number} optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Plz sort by required first, then optional

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.


Note that this specified page cannot be the first two pages nor the last page of the story.

### `price` {string} required
Copy link
Contributor

Choose a reason for hiding this comment

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

Please give one or two examples (e.g. $10 or 10€) so people see how they can specify currencies

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.

Btw I never noticed that it is 10€ instead of €10. nice to know : )

@ychsieh ychsieh merged commit b75d2d7 into ampproject:main Jul 18, 2022
jpettitt pushed a commit to jpettitt/amphtml that referenced this pull request Jul 20, 2022
…ct#38179)

* amp-story-subscriptions doc

* Add validator rules

* update validator tests

* uncommit all validator changes

* Add description for if the optional params are not provided

* change wording to "templated UX"

* Clarification for some attributes

* Fix check link error

* Remove config example and call it out that AMP pages and web stories are using the same way to specify configs.

* Clarification
jpettitt pushed a commit to jpettitt/amphtml that referenced this pull request Jul 21, 2022
…ct#38179)

* amp-story-subscriptions doc

* Add validator rules

* update validator tests

* uncommit all validator changes

* Add description for if the optional params are not provided

* change wording to "templated UX"

* Clarification for some attributes

* Fix check link error

* Remove config example and call it out that AMP pages and web stories are using the same way to specify configs.

* Clarification
Copy link

@Vue-Pu Vue-Pu left a comment

Choose a reason for hiding this comment

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

Approve merge changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants