-
Notifications
You must be signed in to change notification settings - Fork 67
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
[WIP] Allow external scoping #294
Conversation
|
StaticExtraction bool | ||
} | ||
|
||
func Transform(doc *astro.Node, opts TransformOptions) *astro.Node { | ||
shouldScope := len(doc.Styles) > 0 && ScopeStyle(doc.Styles, opts) | ||
shouldScope := opts.ExternalScoping || len(doc.Styles) > 0 && ScopeStyle(doc.Styles, opts) |
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.
The shouldScope
variable is used to determine if the scoped class name should be added to elements. We don't want this to always be true
, because some components do not need the elements to have a scoped class name.
Components that do not have any <style>
s or only have <style global>
do not need the scoped class name. So in this case I think it makes sense for ScopeStyle
to still run, but take opts.ExternalScoping into account for whether not not the CSS should be scoped.
@@ -129,7 +135,7 @@ func preprocessStyle(i int, style *astro.Node, transformOptions transform.Transf | |||
return | |||
} | |||
attrs := wasm_utils.GetAttrs(style) | |||
data, _ := wasm_utils.Await(transformOptions.PreprocessStyle.(js.Value).Invoke(style.FirstChild.Data, attrs)) | |||
data, _ := wasm_utils.Await(transformOptions.PreprocessStyle.(js.Value).Invoke(style.FirstChild.Data, attrs, transformOptions)) |
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.
is this needed? I think that ProcessStyle has access to all of these options on the JS side anyways. Is there something that it needs?
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 so. This was to have the object containing the hash value (e.g. astro-hash1d2c3b4a
).
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.
We talked about this, make sense.
@jonathantneal I'm still tracking this, just FYI. I really want to unblock this and allow power users to bring their own scoping. |
I dug into this over the weekend! While this is definitely one possible route we could take to unblock modern CSS features, the best fix would be to use a better CSS parser that supports all of the features we need out of the box. This PR would mean users need to bring their own logic for scoping, which I can see being a pain. I just opened a PR to swap in the |
Changes
externalScoping
flag.transformOptions
topreprocessStyle
, which should allow PostCSS to apply the styling.Testing
help needed
Docs
help needed