New amp-script samples: multiple fetches, and communicating with an AMP component#4386
New amp-script samples: multiple fetches, and communicating with an AMP component#4386matthiasrohmer merged 6 commits intofuturefrom
Conversation
sebastianbenz
left a comment
sebastianbenz
left a comment
There was a problem hiding this comment.
Great samples! Left a few suggestions how to simplify things.
| <button id="multi-fetch">How slow is our API?</button> | ||
| </amp-script> | ||
| <script id="multi-fetch-script" type="text/plain" target="amp-script"> | ||
| const randomTime = () => Math.floor(Math.random() * 10) + 5; |
There was a problem hiding this comment.
Why make it random? How about making 10 requests where each requests takes 1s longer. This way we can visualize the cut off.
There was a problem hiding this comment.
it could not be random. I think the randomness part makes it more fun to play with. But also, to me, it implies more clearly how that this works - that your code can make mutations until 5 seconds after the last fetch returns... not, say, after you make your last fetch, which is kind of what is implied to me by a linearly increasing set of delays.
Of course, I could keep on making changes for 4 seconds after the last fetch returns, but that seems to me too much.
|
|
||
| function insertText(text) { | ||
| const div = document.createElement('div'); | ||
| div.innerHTML = text; |
There was a problem hiding this comment.
nit: use textContent instead of innerHTML
There was a problem hiding this comment.
whoops. old habit.
| When your script modifies that state variable, the change will propagate to the component. | ||
| --> | ||
| <div id="carousel-sample"> | ||
| <amp-carousel |
There was a problem hiding this comment.
Can you simplify this example? We don't need so much markup just to demonstrate the point. A single image with a binding src value would be sufficient.
There was a problem hiding this comment.
To you and me, <amp-img> is an AMP component. But I think that to most people it's just a replacement for <img>. And what I wanted to show here was that you can use <amp-script> to supercharge an existing AMP component - to give it new features. That people should really try to do this instead of creating components from scratch in JS.
Of course, your example is super clear. In the new documentation, I do something similar - I just show it's possible to use amp-script to modify text in a <p>.
Maybe I should include your example too - showing an image changing - for a simple example showing how amp-script can interact with the DOM via data binding. Because I don't show that. And then I could include my example to show how it's possible to modify a component's behavior. Thinking about it more, I should even put the "Surprise me" button on top of the carousel, so we really see the component has gained a new feature.
There was a problem hiding this comment.
I added the simpler sample showing an image changing. I think this is an improvement - thanks!
| <script id="carousel-script" type="text/plain" target="amp-script"> | ||
| const button = document.getElementById('carousel-button'); | ||
|
|
||
| function gotoRandomSlide() { |
There was a problem hiding this comment.
This also adds too much complexity. Simply switching between two images is enonugh.
The PR to activate the feature in production was merged > 20 days ago (1), but the cut has been delayed due to a P0 Ads issue that I believe was resolved today. It should make it to production before weeks end |
4 participants
Check it out!
I was starting to add a sample for protocol adapters, but then as I tested I realized they're still behind a flag.