📖 protocol adapters: adds documentation#29603
Conversation
samouri
changed the title
samouri
force-pushed
the
fns-list-docs
branch
2 times, most recently
from
0275b62 to
71f7b0d
Compare
dreamofabear
left a comment
dreamofabear
left a comment
There was a problem hiding this comment.
Looks fine but I think (1) the prose can be more concise to match the style of technical documentation and (2) could use some proofreading. @CrystalOnScript might have some good refs.
extensions/amp-list/amp-list.md
Outdated
|
|
||
| 1. **https**: This must refer to a CORS HTTP service. Insecure http is not supported. | ||
| 2. **amp-state**: This is for initializing from amp-state data. See [Initialization from amp-state](#initialization-from-amp-state) for more details. | ||
| 3. **amp-script**: Enables using `<amp-script>` functions as the data source. See [Using amp-script as a data source](#using-amp-script-as-a-data-source) for more details. |
There was a problem hiding this comment.
Nit: amp-state vs. <amp-script> is inconsistent.
There was a problem hiding this comment.
I've never figured out what we prefer in documentation. I tend to use angle brackets, but not everyone does. @CrystalOnScript , is there a standard?
There was a problem hiding this comment.
Also, looking at this again - can you restate "this is for initializing from amp-state data" more clearly?
morsssss
left a comment
morsssss
left a comment
There was a problem hiding this comment.
Thanks for writing all this documentation! I'm excited about this. I just think some things need one more draft. (Writing is hard...)
extensions/amp-list/amp-list.md
Outdated
| See below for an example: | ||
|
|
||
| ```html | ||
| <amp-script id="fns" script="local-script" nodom></amp-script> |
There was a problem hiding this comment.
The id is arbitrary. It just needs to match what ends up going in the src of the <amp-list>'s src
There was a problem hiding this comment.
Can you choose something more descriptive than "fns"?
(I imagine it's short for "functions", but even more descriptive than that would be great)
There was a problem hiding this comment.
I'm not sure if you are thinking it should be named by its contents or in a more meta way. E.g. either script-getRemoteData, or exported-functions.
There was a problem hiding this comment.
I think something descriptive would be best. Like dataFunctions, apiShim, etc.
extensions/amp-list/amp-list.md
Outdated
|
|
||
| 1. **https**: This must refer to a CORS HTTP service. Insecure http is not supported. | ||
| 2. **amp-state**: This is for initializing from amp-state data. See [Initialization from amp-state](#initialization-from-amp-state) for more details. | ||
| 3. **amp-script**: Enables using `<amp-script>` functions as the data source. See [Using amp-script as a data source](#using-amp-script-as-a-data-source) for more details. |
There was a problem hiding this comment.
I've never figured out what we prefer in documentation. I tend to use angle brackets, but not everyone does. @CrystalOnScript , is there a standard?
|
FYI I think documentation rollout on amp.dev follows our release schedule, so no need to wait before merge. |
morsssss
left a comment
morsssss
left a comment
There was a problem hiding this comment.
only spotted two things, one which is a teeny nit, and one which will matter!
morsssss
left a comment
morsssss
left a comment
There was a problem hiding this comment.
Thanks for all your work on the documentation! I'm pretty excited about this feature, and I know I'm not the only one.
* protocol adapters: update documentation * amp-script docs * typo * update docs * document nodom * morning cleanup * gulp prettify --fix * fix merge conflict * address feedback * period word * remove more words and linkify * typo * nit HTTP * some more small updates * morrrsssss updates
4 participants
Followup to #29541
I plan on holding off on merging this until the experiment is turned on.