From 3065aeb231e4f769a6919a0937521d54e15f5bc0 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Tue, 10 Mar 2020 14:29:37 -0400 Subject: [PATCH 01/11] amp-list: add example for initialization from amp-state --- examples/amp-list.state.html | 81 ++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 examples/amp-list.state.html diff --git a/examples/amp-list.state.html b/examples/amp-list.state.html new file mode 100644 index 000000000000..f24d95be31fc --- /dev/null +++ b/examples/amp-list.state.html @@ -0,0 +1,81 @@ + + + + + + + + + + + + + + + +
+ Refresh All Lists +
+ +

Static amp-list, inlined state

+ + + + +

Static amp-list, async state

+ + + + +

Paging amp-list

+ +
+ Show more +
+ + + + + + + + + From 6c83ebb4bacc69443820f1aed5c4d138ba9eb05b Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Tue, 10 Mar 2020 14:47:28 -0400 Subject: [PATCH 02/11] improve example, and add amp-state to allowed protocols --- examples/amp-list.state.html | 41 +++++++------------ .../amp-list/0.1/test/validator-amp-list.html | 4 ++ .../amp-list/0.1/test/validator-amp-list.out | 36 ++++++++-------- .../amp-list/validator-amp-list.protoascii | 1 + 4 files changed, 39 insertions(+), 43 deletions(-) diff --git a/examples/amp-list.state.html b/examples/amp-list.state.html index f24d95be31fc..a8371d1684db 100644 --- a/examples/amp-list.state.html +++ b/examples/amp-list.state.html @@ -25,52 +25,39 @@ Refresh All Lists -

Static amp-list, inlined state

- +

Static amp-list, inlined state. + +

+

Static amp-list, async state

- +

Paging amp-list

- -
+ +
Show more
- - + + + +... +``` + +In several cases, we may need the `` to resize on user interaction. For example, when the `` contains an amp-accordion that a user may tap on, when the contents of the `` change size due to bound CSS classes, or when the number of items inside an `` changes due to a bound `[src]` attribute. The `changeToLayoutContainer` action handles this by changing the amp list to `layout="CONTAINER"` when triggering this action. See the following example: + ## Attributes ### src (required) From 649fe6e3b88774a8caeddf5e3d06c08310a2f7a9 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Tue, 10 Mar 2020 16:17:49 -0400 Subject: [PATCH 04/11] remove feature flag --- extensions/amp-list/0.1/amp-list.js | 5 +---- extensions/amp-list/0.1/test/test-amp-list.js | 21 +------------------ tools/experiments/experiments-config.js | 5 ----- 3 files changed, 2 insertions(+), 29 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index 364b724f1d8b..01e45175e2b9 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -352,10 +352,7 @@ export class AmpList extends AMP.BaseElement { * @private */ isAmpStateSrc_(src) { - return ( - isExperimentOn(this.win, 'amp-list-init-from-state') && - startsWith(src, AMP_STATE_URI_SCHEME) - ); + return startsWith(src, AMP_STATE_URI_SCHEME) } /** diff --git a/extensions/amp-list/0.1/test/test-amp-list.js b/extensions/amp-list/0.1/test/test-amp-list.js index 7d35dc5f81e3..ee066976c015 100644 --- a/extensions/amp-list/0.1/test/test-amp-list.js +++ b/extensions/amp-list/0.1/test/test-amp-list.js @@ -20,10 +20,6 @@ import {AmpEvents} from '../../../../src/amp-events'; import {AmpList} from '../amp-list'; import {Deferred} from '../../../../src/utils/promise'; import {Services} from '../../../../src/services'; -import { - resetExperimentTogglesForTesting, - toggleExperiment, -} from '../../../../src/experiments'; describes.repeated( 'amp-list', @@ -809,9 +805,7 @@ describes.repeated( ); }); - it('"amp-state:" uri should skip rendering and emit an error', () => { - toggleExperiment(win, 'amp-list-init-from-state', true); - + it('"amp-state:" uri should skip rendering and emit an error', () => { const ampStateEl = doc.createElement('amp-state'); ampStateEl.setAttribute('id', 'okapis'); const ampStateJson = doc.createElement('script'); @@ -1230,24 +1224,14 @@ describes.repeated( }); describe('Using amp-state: protocol', () => { - const experimentName = 'amp-list-init-from-state'; - beforeEach(() => { - resetExperimentTogglesForTesting(win); element = createAmpListElement(); element.setAttribute('src', 'amp-state:okapis'); element.toggleLoading = () => {}; list = createAmpList(element); }); - it('should throw an error if used without the experiment enabled', async () => { - const errorMsg = /Invalid value: amp-state:okapis/; - expectAsyncConsoleError(errorMsg); - expect(list.layoutCallback()).to.eventually.throw(errorMsg); - }); - it('should throw error if there is no associated amp-state el', async () => { - toggleExperiment(win, experimentName, true); bind.getStateAsync = () => Promise.reject(); const errorMsg = /element with id 'okapis' was not found/; @@ -1256,7 +1240,6 @@ describes.repeated( }); it('should log an error if amp-bind was not included', async () => { - toggleExperiment(win, experimentName, true); Services.bindForDocOrNull.returns(Promise.resolve(null)); const ampStateEl = doc.createElement('amp-state'); @@ -1272,7 +1255,6 @@ describes.repeated( }); it('should render a list using local data', async () => { - toggleExperiment(win, experimentName, true); bind.getStateAsync = () => Promise.resolve({items: [1, 2, 3]}); const ampStateEl = doc.createElement('amp-state'); @@ -1292,7 +1274,6 @@ describes.repeated( }); it('should render a list using async data', async () => { - toggleExperiment(win, experimentName, true); const {resolve, promise} = new Deferred(); bind.getStateAsync = () => promise; diff --git a/tools/experiments/experiments-config.js b/tools/experiments/experiments-config.js index 8df3366bbef1..561d20006b4f 100644 --- a/tools/experiments/experiments-config.js +++ b/tools/experiments/experiments-config.js @@ -107,11 +107,6 @@ export const EXPERIMENTS = [ name: 'Allows the new lightbox experience to be used in A4A (prototype).', spec: 'https://github.com/ampproject/amphtml/issues/7743', }, - { - id: 'amp-list-init-from-state', - name: 'Allows amp-list to initialize off of amp-state', - spec: 'https://github.com/ampproject/amphtml/issues/26009', - }, { id: 'amp-playbuzz', name: 'AMP extension for playbuzz items (launched)', From cef1194e606646443dba6194535cd10fa3582441 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Tue, 10 Mar 2020 16:19:27 -0400 Subject: [PATCH 05/11] unique ids --- examples/amp-list.state.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/examples/amp-list.state.html b/examples/amp-list.state.html index a8371d1684db..26f83d259fc7 100644 --- a/examples/amp-list.state.html +++ b/examples/amp-list.state.html @@ -21,7 +21,7 @@ -
+
Refresh All Lists
@@ -36,14 +36,14 @@

Static amp-list, inlined state.

Static amp-list, async state

- +

Paging amp-list

- +
Show more
From 3f07676c7e2656dbbab798f762bdea4f3bb73156 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Tue, 10 Mar 2020 16:32:47 -0400 Subject: [PATCH 06/11] lint --- extensions/amp-list/0.1/amp-list.js | 3 +-- extensions/amp-list/0.1/test/test-amp-list.js | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index 01e45175e2b9..c3c603ae5906 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -60,7 +60,6 @@ import { setupJsonFetchInit, } from '../../../src/utils/xhr-utils'; import {isArray, toArray} from '../../../src/types'; -import {isExperimentOn} from '../../../src/experiments'; import {px, setStyles, toggle} from '../../../src/style'; import {setDOM} from '../../../third_party/set-dom/set-dom'; import {startsWith} from '../../../src/string'; @@ -352,7 +351,7 @@ export class AmpList extends AMP.BaseElement { * @private */ isAmpStateSrc_(src) { - return startsWith(src, AMP_STATE_URI_SCHEME) + return startsWith(src, AMP_STATE_URI_SCHEME); } /** diff --git a/extensions/amp-list/0.1/test/test-amp-list.js b/extensions/amp-list/0.1/test/test-amp-list.js index ee066976c015..a6edc1ed1f04 100644 --- a/extensions/amp-list/0.1/test/test-amp-list.js +++ b/extensions/amp-list/0.1/test/test-amp-list.js @@ -805,7 +805,7 @@ describes.repeated( ); }); - it('"amp-state:" uri should skip rendering and emit an error', () => { + it('"amp-state:" uri should skip rendering and emit an error', () => { const ampStateEl = doc.createElement('amp-state'); ampStateEl.setAttribute('id', 'okapis'); const ampStateJson = doc.createElement('script'); From fe352c48895f1cc96be716faa45a2a65ed26e14e Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Wed, 11 Mar 2020 13:37:14 -0400 Subject: [PATCH 07/11] format amp-list.md --- extensions/amp-list/amp-list.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/amp-list/amp-list.md b/extensions/amp-list/amp-list.md index 402274e705a7..cabd493123c8 100644 --- a/extensions/amp-list/amp-list.md +++ b/extensions/amp-list/amp-list.md @@ -229,9 +229,9 @@ would have been retrieved from an endpoint. For example, ```html ... From 34e24ddb82d4d891591bd0c7b502468bc2127a0e Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Wed, 11 Mar 2020 16:45:21 -0400 Subject: [PATCH 08/11] remove accidental additions --- extensions/amp-list/amp-list.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/extensions/amp-list/amp-list.md b/extensions/amp-list/amp-list.md index cabd493123c8..b6d3369b19a2 100644 --- a/extensions/amp-list/amp-list.md +++ b/extensions/amp-list/amp-list.md @@ -237,8 +237,6 @@ would have been retrieved from an endpoint. For example, ... ``` -In several cases, we may need the `` to resize on user interaction. For example, when the `` contains an amp-accordion that a user may tap on, when the contents of the `` change size due to bound CSS classes, or when the number of items inside an `` changes due to a bound `[src]` attribute. The `changeToLayoutContainer` action handles this by changing the amp list to `layout="CONTAINER"` when triggering this action. See the following example: - ## Attributes ### src (required) From 5e92cc78e95c7008c7c121069fd0760c3a02c07a Mon Sep 17 00:00:00 2001 From: Jake F Date: Thu, 12 Mar 2020 15:48:14 -0400 Subject: [PATCH 09/11] address choumx feedback --- .../global-configs/canary-config.json | 1 + build-system/global-configs/prod-config.json | 1 + examples/amp-list.state.html | 1 - extensions/amp-list/0.1/amp-list.js | 6 ++++- extensions/amp-list/0.1/test/test-amp-list.js | 19 +++++++++++++++ extensions/amp-list/amp-list.md | 23 +++++++++++++++---- 6 files changed, 44 insertions(+), 7 deletions(-) diff --git a/build-system/global-configs/canary-config.json b/build-system/global-configs/canary-config.json index a2322ad9f52e..da20a87a7414 100644 --- a/build-system/global-configs/canary-config.json +++ b/build-system/global-configs/canary-config.json @@ -15,6 +15,7 @@ "amp-auto-ads-adsense-holdout": 0.1, "amp-auto-ads-no-op-experiment": 0.05, "amp-consent-restrict-fullscreen": 1, + "amp-list-init-from-state": 1, "amp-mega-menu": 1, "amp-nested-menu": 1, "amp-playbuzz": 1, diff --git a/build-system/global-configs/prod-config.json b/build-system/global-configs/prod-config.json index 9c09aa65d6bf..00c9f64479b0 100644 --- a/build-system/global-configs/prod-config.json +++ b/build-system/global-configs/prod-config.json @@ -15,6 +15,7 @@ "amp-auto-ads-adsense-holdout": 0.1, "amp-auto-ads-no-op-experiment": 0.05, "amp-consent-restrict-fullscreen": 1, + "amp-list-init-from-state": 1, "amp-mega-menu": 1, "amp-nested-menu": 1, "amp-playbuzz": 1, diff --git a/examples/amp-list.state.html b/examples/amp-list.state.html index 26f83d259fc7..8ba27452e8f4 100644 --- a/examples/amp-list.state.html +++ b/examples/amp-list.state.html @@ -20,7 +20,6 @@ -
Refresh All Lists
diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index c3c603ae5906..364b724f1d8b 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -60,6 +60,7 @@ import { setupJsonFetchInit, } from '../../../src/utils/xhr-utils'; import {isArray, toArray} from '../../../src/types'; +import {isExperimentOn} from '../../../src/experiments'; import {px, setStyles, toggle} from '../../../src/style'; import {setDOM} from '../../../third_party/set-dom/set-dom'; import {startsWith} from '../../../src/string'; @@ -351,7 +352,10 @@ export class AmpList extends AMP.BaseElement { * @private */ isAmpStateSrc_(src) { - return startsWith(src, AMP_STATE_URI_SCHEME); + return ( + isExperimentOn(this.win, 'amp-list-init-from-state') && + startsWith(src, AMP_STATE_URI_SCHEME) + ); } /** diff --git a/extensions/amp-list/0.1/test/test-amp-list.js b/extensions/amp-list/0.1/test/test-amp-list.js index a6edc1ed1f04..7d35dc5f81e3 100644 --- a/extensions/amp-list/0.1/test/test-amp-list.js +++ b/extensions/amp-list/0.1/test/test-amp-list.js @@ -20,6 +20,10 @@ import {AmpEvents} from '../../../../src/amp-events'; import {AmpList} from '../amp-list'; import {Deferred} from '../../../../src/utils/promise'; import {Services} from '../../../../src/services'; +import { + resetExperimentTogglesForTesting, + toggleExperiment, +} from '../../../../src/experiments'; describes.repeated( 'amp-list', @@ -806,6 +810,8 @@ describes.repeated( }); it('"amp-state:" uri should skip rendering and emit an error', () => { + toggleExperiment(win, 'amp-list-init-from-state', true); + const ampStateEl = doc.createElement('amp-state'); ampStateEl.setAttribute('id', 'okapis'); const ampStateJson = doc.createElement('script'); @@ -1224,14 +1230,24 @@ describes.repeated( }); describe('Using amp-state: protocol', () => { + const experimentName = 'amp-list-init-from-state'; + beforeEach(() => { + resetExperimentTogglesForTesting(win); element = createAmpListElement(); element.setAttribute('src', 'amp-state:okapis'); element.toggleLoading = () => {}; list = createAmpList(element); }); + it('should throw an error if used without the experiment enabled', async () => { + const errorMsg = /Invalid value: amp-state:okapis/; + expectAsyncConsoleError(errorMsg); + expect(list.layoutCallback()).to.eventually.throw(errorMsg); + }); + it('should throw error if there is no associated amp-state el', async () => { + toggleExperiment(win, experimentName, true); bind.getStateAsync = () => Promise.reject(); const errorMsg = /element with id 'okapis' was not found/; @@ -1240,6 +1256,7 @@ describes.repeated( }); it('should log an error if amp-bind was not included', async () => { + toggleExperiment(win, experimentName, true); Services.bindForDocOrNull.returns(Promise.resolve(null)); const ampStateEl = doc.createElement('amp-state'); @@ -1255,6 +1272,7 @@ describes.repeated( }); it('should render a list using local data', async () => { + toggleExperiment(win, experimentName, true); bind.getStateAsync = () => Promise.resolve({items: [1, 2, 3]}); const ampStateEl = doc.createElement('amp-state'); @@ -1274,6 +1292,7 @@ describes.repeated( }); it('should render a list using async data', async () => { + toggleExperiment(win, experimentName, true); const {resolve, promise} = new Deferred(); bind.getStateAsync = () => promise; diff --git a/extensions/amp-list/amp-list.md b/extensions/amp-list/amp-list.md index b6d3369b19a2..d05f7e6398b5 100644 --- a/extensions/amp-list/amp-list.md +++ b/extensions/amp-list/amp-list.md @@ -222,19 +222,32 @@ In several cases, we may need the `` to resize on user interaction. Fo ### Initialization from amp-state -In some cases, it may be desirable to have your `` component initialize off of `` rather than from a json endpoint. -You may do that by utilizing the `amp-state:` protocol in the `src` attribute. The data in state must follow the same rules as the json that -would have been retrieved from an endpoint. For example, +For cases where you can server-side-render the JSON for an ``, it may be +desirable to have your `` component initialize off of `` rather +than from a JSON endpoint. This allows the component to render faster by skipping the fetch, +but also means the data may be stale if served from the AMP Cache. + +You may enable this by following two steps: + +1. Add the [amp-bind](https://amp.dev/documentation/components/amp-bind/) script to the `` of your document. +2. Utilize an `amp-state:` protocol in the `src` attribute. The data in state + must follow the same rules as the JSON that would have been retrieved from an endpoint. + +For example, ```html - + -... + + + ``` ## Attributes From 427d1277eb9e1e671409aafeda516f63dd72a956 Mon Sep 17 00:00:00 2001 From: Jake F Date: Mon, 16 Mar 2020 11:19:56 -0400 Subject: [PATCH 10/11] revert experiments-config.js --- tools/experiments/experiments-config.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/experiments/experiments-config.js b/tools/experiments/experiments-config.js index 561d20006b4f..8df3366bbef1 100644 --- a/tools/experiments/experiments-config.js +++ b/tools/experiments/experiments-config.js @@ -107,6 +107,11 @@ export const EXPERIMENTS = [ name: 'Allows the new lightbox experience to be used in A4A (prototype).', spec: 'https://github.com/ampproject/amphtml/issues/7743', }, + { + id: 'amp-list-init-from-state', + name: 'Allows amp-list to initialize off of amp-state', + spec: 'https://github.com/ampproject/amphtml/issues/26009', + }, { id: 'amp-playbuzz', name: 'AMP extension for playbuzz items (launched)', From f07127f005ddb02ce60d36df11ef75cf4a4f3ec8 Mon Sep 17 00:00:00 2001 From: Jake F Date: Mon, 16 Mar 2020 11:30:04 -0400 Subject: [PATCH 11/11] ben rewrite --- extensions/amp-list/amp-list.md | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/extensions/amp-list/amp-list.md b/extensions/amp-list/amp-list.md index d05f7e6398b5..d4867a1bea87 100644 --- a/extensions/amp-list/amp-list.md +++ b/extensions/amp-list/amp-list.md @@ -222,18 +222,17 @@ In several cases, we may need the `` to resize on user interaction. Fo ### Initialization from amp-state -For cases where you can server-side-render the JSON for an ``, it may be -desirable to have your `` component initialize off of `` rather -than from a JSON endpoint. This allows the component to render faster by skipping the fetch, -but also means the data may be stale if served from the AMP Cache. +In most cases, you’ll probably want to have `` request JSON from a server. But `` can also use JSON you’ve included in an ``, right there in your HTML! This means rendering can occur without an additional server call, although, of course, if your page is served from an AMP cache, the data may not be fresh. -You may enable this by following two steps: +Here’s how to have `` render from an ``: -1. Add the [amp-bind](https://amp.dev/documentation/components/amp-bind/) script to the `` of your document. -2. Utilize an `amp-state:` protocol in the `src` attribute. The data in state - must follow the same rules as the JSON that would have been retrieved from an endpoint. +1. Add the [amp-bind](https://amp.dev/documentation/components/amp-bind/) script to your document's ``. +2. Use the `amp-state:` protocol in your ``’s src attribute, like this: + `` -For example, +Note that `` treats your JSON in the same way whether it’s requested from your server or pulled from a state variable. The format required doesn’t change. + +See below for a full example, ```html