Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 [Story performance] Set up story prestyle #35829

Closed
wants to merge 18 commits into from

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Aug 26, 2021

Set up visual tests and CSS deployment for prestyle.

@mszylkowski mszylkowski self-assigned this Aug 26, 2021
@mszylkowski mszylkowski marked this pull request as draft August 26, 2021 20:09
@mszylkowski mszylkowski added this to In progress in wg-stories Performance via automation Aug 26, 2021
@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Aug 26, 2021
@mszylkowski mszylkowski marked this pull request as ready for review August 27, 2021 15:56
@@ -75,6 +75,13 @@ const cssEntryPoints = [
outCss: 'amp-story-player-shadow-v0.css',
append: false,
},
{
// Story CSS used to layout story before JS loads.
path: 'amp-story-prestyle.css',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of "prestyle" since it's technically all the styles. We don't even need to include the CSS in the JS bundle when this is loaded, and this is def something we should look into very soon.

I'm open to anything, but since the script is amp-story-1.0.js, should we do amp-story-1.0.css?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG to change the name since we're including the CSS styles of amp-story, but it would be confusing to name it amp-story-1.0.css since there's already a CSS output at https://cdn.ampproject.org/v0/amp-story-1.0.css. I think it might even clash the paths so we shouldn't do that. What about amp-story-v1.css that matches all the other exported CSS files in this list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly think the correct way forward might be to use amp-story-1.0.css, not even generate a new target, and down the line remove it from the JS bundle. From our convo with @jridgewell this should be super easy to do

}
.i-amphtml-notbuilt > * {
display: initial;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK for this boilerplate PR but as we discussed, in the next PR this should be a @import 'amp-story.css'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think some of these styles will still be needed (they are not in amp-story.css) but on the next PR I'll import the CSS and deal with it.

<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Poppins:wght@400;700&display=swap" rel="stylesheet">
<link rel="stylesheet" href="/css/amp-story-prestyle.css">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be https://cdn.ampproject.org/v0/amp-story-1.0.css and get automatically rewritten. Can you look into what that involves and/or create a ticket?

@mszylkowski mszylkowski marked this pull request as draft September 3, 2021 16:57
@mszylkowski mszylkowski removed the request for review from rileyajones September 3, 2021 16:58
wg-stories Performance automation moved this from In progress to Done Sep 10, 2021
wg-stories Sprint automation moved this from In progress to Done Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants