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

initialize amp-list from amp-state, finishing touches #27180

Merged
merged 11 commits into from
Mar 17, 2020

Conversation

samouri
Copy link
Member

@samouri samouri commented Mar 10, 2020

summary

  • adds an example to examples/
  • adds documentation to amp-list.md
  • adds amp-state: protocol to list of allowed ones in the validator.
  • adds the feature flag to canary/prod, turned up to 1.

@samouri samouri changed the title Example and doc initialize amp-list from amp-state, finishing touches Mar 10, 2020
@samouri samouri marked this pull request as ready for review March 10, 2020 20:20
@amp-owners-bot amp-owners-bot bot requested a review from JonEleven March 10, 2020 20:20
@amp-owners-bot
Copy link

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-list/0.1/test/validator-amp-list.html
extensions/amp-list/0.1/test/validator-amp-list.out
extensions/amp-list/validator-amp-list.protoascii

Refresh All Lists
</div>

<h3>Static amp-list, inlined state.

Choose a reason for hiding this comment

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

You can consider adding this to examples/bind/list.amp.html. Also feel free to reorganize these example files if you'd like. They're pretty messy.

Copy link
Member Author

Choose a reason for hiding this comment

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

It feels as though it equally belongs with amp-list and amp-bind, but potentially more so with amp-list. Mind if I move all of the amp-list examples to examples/amp-list/*.html after this PR goes through?

extensions/amp-list/0.1/amp-list.js Outdated Show resolved Hide resolved
@@ -220,6 +220,23 @@ In several cases, we may need the `<amp-list>` to resize on user interaction. Fo
</amp-list>
```

### Initialization from amp-state

In some cases, it may be desirable to have your `<amp-list>` component initialize off of `<amp-state>` rather than from a json endpoint.

Choose a reason for hiding this comment

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

Let's be more prescriptive here to help developers do the right thing. When is this a good idea?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some more detail, let me know if you think it can be improved further!

extensions/amp-list/amp-list.md Outdated Show resolved Hide resolved
extensions/amp-list/amp-list.md Outdated Show resolved Hide resolved
}
</script>
</amp-state>
<amp-list src="amp-state:listExample">...</amp-list>

Choose a reason for hiding this comment

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

Let's make this example as simple as possible but also complete enough to be copy/pasted.

Nit: s/listExample/localState?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to double check that this works actually (via copy/paste)

@@ -98,6 +98,7 @@ tags: { # <amp-list> with mandatory src and/or [src] attr
mandatory_anyof: "['src','[src]','data-amp-bind-src']"
value_url: {
protocol: "https"
protocol: "amp-state"

Choose a reason for hiding this comment

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

@samouri samouri merged commit 5348681 into ampproject:master Mar 17, 2020
@samouri samouri deleted the example-and-doc branch March 17, 2020 01:21
@samouri
Copy link
Member Author

samouri commented Mar 17, 2020

@morsssss: I submitted with all your changes included. If I missed anything or you change your mind about the wording of the docs, feel free to either ping me or open a PR and attach me as a reviewer!

@morsssss
Copy link
Contributor

thanks @samouri !

amaltas added a commit that referenced this pull request Mar 18, 2020
* cl/301228504 Revision bump for #27083

* cl/301306664 Revision bump for #27180

* Update validator-amp-list.out

* Update validator-amp-list.out

* Update validator-amp-list.out
twintwox pushed a commit to twintwox/amphtml that referenced this pull request Mar 24, 2020
* amp-list: add example for initialization from amp-state

* improve example, and add amp-state to allowed protocols

* add documentation

* remove feature flag

* unique ids

* lint

* format amp-list.md

* remove accidental additions

* address choumx feedback

* revert experiments-config.js

* ben rewrite
twintwox pushed a commit to twintwox/amphtml that referenced this pull request Mar 24, 2020
* cl/301228504 Revision bump for ampproject#27083

* cl/301306664 Revision bump for ampproject#27180

* Update validator-amp-list.out

* Update validator-amp-list.out

* Update validator-amp-list.out
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