📖 [amp-story-player] Initial docs#26606
Conversation
|
Hey @alanorozco, these files were changed:
Hey @wassgha, these files were changed:
|
|
Friendly ping |
CrystalOnScript
left a comment
There was a problem hiding this comment.
I've got a couple of big things about this one:
-
I really don't think this information should be inside the
amp-storyfolder. Everything in theamp-storyfolder is either the<amp-story>component, or child components of<amp-story>and must be on AMP valid pages.<amp-story-player>is the exact opposite of this. I think we need to seriously reconsider the structure of this documentation. -
It is very unclear to me if I can use this component/viewer on AMP pages or if it is strictly for non-AMP pages.
-
The fact that we can use this in non-AMP pages makes me question further if it should be in the
extensionsfolder. I highlighted using the extension-doc.template.md but is it an extension of AMP?
42e7d05 to
fb31b40
Compare
|
Ready for another review @CrystalOnScript 👍 |
CrystalOnScript
left a comment
There was a problem hiding this comment.
Thanks for migrating it into spec!
78e98d2 to
a56a217
Compare
|
Thanks @CrystalOnScript for the detailed review! /to @dvoytenko for owners approval |
|
|
||
| ### Attributes | ||
|
|
||
| The inline width and height ensures that the player will not cause any jumps in your website while the script is being loaded. Feel free to modify the values, but we recommend maintaining a 3:5 aspect ratio. |
There was a problem hiding this comment.
cc @hongwei1990 to make sure this is right
a56a217 to
806cecb
Compare
|
|
||
| ```html | ||
| <head> | ||
| <script |
There was a problem hiding this comment.
Does prettier force this to be formatted like this? Can we just make them one line for the script and one line for the style?
There was a problem hiding this comment.
Yep I'm not happy with it either, but prettier forces this style...
806cecb to
4f0e249
Compare
|
PTAL @newmuis |
|
Friendly ping @dvoytenko for OWNERS approval in |
* master: (41 commits) custom-element: Minor test improvements (ampproject#26923) amp-pixel: Minor test improvements (ampproject#26918) viewer: Minor test improvements (ampproject#26906) dom: Minor test improvements (ampproject#26913) amp-action: Support whitelist lookup in AmpDocShadow (ampproject#26684) ✨ Update amp-access-scroll (ampproject#26810) 🚀 Remove doc css and base css from ESM build (ampproject#26889) 📖 [amp-story-player] Initial docs (ampproject#26606) Amp consent restrict fullscreen prod flag (ampproject#26909) 📖 Clarify SXG duration minimum (ampproject#26890) Improve test vendor requests macros (ampproject#26828) 🚀 Move scroll left and top macros out of url-replacement-impl (ampproject#25594) Update consent string maximum size to 200 bytes (ampproject#26741) ✨[amp-story-player] Adds tap-to-next/previous story (ampproject#26865) update owners file with correct syntax (ampproject#26899) amp-sticky-ad: Fix unit test (ampproject#26855) Add performance metrics to README (ampproject#26891) 🐛 Bug fix: check links test (ampproject#26739) ✨Idealmedia uniq ad (ampproject#25838) 📦 Update dependency jsdom to v16.2.0 (ampproject#26591) ...
Adds initial documentation for the new
amp-story-player.I2I #24539
Tracker #26308