🖍 [amp-story-player] Adds css for pre-fetch experience #26557
Conversation
examples/amp-story/player.html
Outdated
| @@ -2,6 +2,7 @@ | |||
| <head> | |||
| <title>Story Player (Non-AMP)</title> | |||
| <script async src="../../dist/amp-story-player.js"></script> | |||
| <link href="../../build/css/amp-story-player-out.css" rel='stylesheet' type='text/css'> | |||
There was a problem hiding this comment.
Is it possible to get a URL that works on our machines during development, and that we can host on firebase using the existing scripts?
What's going to be the final production URL to load this stylesheet?
There was a problem hiding this comment.
Final production URL with this configuration would be https://cdn.ampproject.org/amp-story-player-out.css.
I agree with your comment above; let's rename this to something more user-friendly.
There was a problem hiding this comment.
The url in this example works on our machines during development, I am not sure about hosting on firebase.
For context, this is currently the only way we can export a css file without it being a part of an AMP extension (otherwise we could do it through bundles.config.js, but since this is under src/ and not under extensions/it won't be picked up by the build system), video-autoplay is another example that does this.
If we want it to be hosted on firebase I see 3 options:
- Copy + paste the file manually from the local
amphtml/**/build/*to afirebase/build/dir. - Modify the firebase script to also include files in the
build/*directory. - Create an AMP extension (which we will eventually) and use its css file instead of this one.
There was a problem hiding this comment.
I think we just need to use the output from dist for this to work. For example, right now, I can hit http://stamp-gmajoulet.firebaseapp.com/dist/video-autoplay-out.css. I would still expect the relative URLs to work just fine, even on Firebase.
There was a problem hiding this comment.
Oh nice! I should've checked this before 😅
examples/amp-story/player.html
Outdated
| @@ -2,6 +2,7 @@ | |||
| <head> | |||
| <title>Story Player (Non-AMP)</title> | |||
| <script async src="../../dist/amp-story-player.js"></script> | |||
| <link href="../../build/css/amp-story-player-out.css" rel='stylesheet' type='text/css'> | |||
There was a problem hiding this comment.
Final production URL with this configuration would be https://cdn.ampproject.org/amp-story-player-out.css.
I agree with your comment above; let's rename this to something more user-friendly.
src/amp-story-player.js
Outdated
| @@ -36,7 +36,7 @@ const LoadStateClass = { | |||
|
|
|||
| /** @const {string} */ | |||
| const CSS = ` | |||
| :host { all: initial; display: block; border-radius: 0 !important; width: 405px; height: 720px; overflow: auto; } | |||
| :host { all: initial; display: block; border-radius: 0 !important; width: 320px; height: 480px; overflow: auto; } | |||
There was a problem hiding this comment.
Unrelated suggestion: You can use a separate CSS file that's compiled inline into here as well. See src/service/video/install-autoplay-styles.js for example.
There was a problem hiding this comment.
Hmm, correct me if I'm wrong but it looks like this would only work for AMP docs? Since this is a JS library (non-AMP) I don't think we can use this.
There was a problem hiding this comment.
This part still works:
import {cssText} from '../../../build/video-autoplay.css';Regardless of delivery mechanism you can build the cssText string that way to get all other CSS tooling.
There was a problem hiding this comment.
Got it, will address in a follow-up. Thanks for the pointer!
There was a problem hiding this comment.
Following up in #26604
ampprojectbot
added
PR use: In Beta / Experimental
and removed
PR use: In Beta / Experimental
labels
amp-story-playerwhich will set a placeholder specified by the publisher (--story-player-poster).Partial for #24539 #26308