-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: configuration for css inlining behavior #6659
Conversation
🦋 Changeset detectedLatest commit: a73490c The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
55e1a14
to
350446c
Compare
a8fa467
to
4475b42
Compare
37500b5
to
3114f61
Compare
inlineStylesheet
configuration option
@matthewp could I get some feedback on this? |
37500b5
to
fb8b77c
Compare
!preview css-inline |
|
16314db
to
b870620
Compare
7398995
to
033831c
Compare
@lilnasy Heads up that we are planning our next |
@matthewp it's all done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocked as this is a minor. Block will be released when the next release occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really great. I appreciate the amount of work this took and how thorough you were with testing and everything else.
@matthewp I found a mistake in one of the tests I wrote. I'd prefer to rebase and keep the commit history clean, but I'm hesitating because the review is done. Is rebasing now fine? |
When PRs get merged they are squashed into a single commit. So you can just push new commits. |
c207c40
to
4d2cb94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this! I have a few nits/questions below that I'd love your thoughts on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file should be named with .test.js
. About the fixtures, could they all share a single project at ./fixtures/css-inline-stylesheets
instead of ./fixtures/css-inline-stylesheet/(always|auto|never)
? They seem to contain the same files and it would be easier to maintain if they're combined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discord, there's a weird caching issue when sharing a single fixture, so keeping as is for now. For the test name, I handled it at #6983
Content collections reuses build data across multiple fixture.builds, even though a configuration change may have changed it. Duplicating fixtures avoids usage of the stale cache. https://cdn.discordapp.com/attachments/1039830843440504872/1097795182340092024/Screenshot_87_colored.png
Chrome, but not Safari or Firefox, is slower to match rules when they are split across multiple files or style tags. https://nolanlawson.com/2022/06/22/style-scoping-versus-shadow-dom-which-is-fastest/ Having the abiility to inline stylesheets opens us up to this optimization. Ideally, it would extend to propagated styles, but that ended up being a rabbit hole.
Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
RFC: withastro/roadmap#343
Major points of review:
<link>
(external) and<style>
(inline) tagsTesting
Docs
Docs changes will be required regarding the addition of a configuration option.