Skip to content

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
@amp-owners-bot amp-owners-bot bot requested review from Enriqe and estherkim August 26, 2021 20:09
@mszylkowski mszylkowski marked this pull request as draft August 26, 2021 20:09
@mszylkowski mszylkowski requested a review from gmajoulet August 27, 2021 15:50
@mszylkowski mszylkowski marked this pull request as ready for review August 27, 2021 15:56
@amp-owners-bot amp-owners-bot bot requested a review from rileyajones August 27, 2021 15:56
},
{
// 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

@mszylkowski mszylkowski Aug 31, 2021

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants