-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add keyframes
support for scoped style ?
#245
Comments
I'm looking at the code responsible for this and it's hurting my head https://github.com/sveltejs/svelte/blob/master/src/generators/shared/css/transform.js :( There's no real parsing of the CSS, it's just a bunch of regular expressions. This is kind of icky because things like I don't know how robust we ought to make this. I believe one workaround now (without any changes to Svelte) would be to just use Other than that, I don't know what to suggest, because I suspect we don't want to bring in a whole CSS parsing and processing library. The least-impact solution might be to just put a warning in the docs that you need to use |
@jihchi Using |
My initial thought was that we probably should use a dedicated CSS parser (most likely PostCSS) because that would make it a lot easier to do things like minification/optimisation in a sourcemap-aware way: #8 (comment). Because Svelte only runs at build time, there isn't the same pressure to be lean. The code in that file is copied straight from Ractive, where there was pressure to keep things compact as in many cases the code runs in the browser. It was only really intended as a stopgap measure. That said, FWIW, in the past when I've encountered this problem (with Ractive projects) I've reluctantly included a separate .css file with my keyframe declarations. Is a cop-out, but it works around the issue until we fix it properly. |
Thanks for the help / workaround. @Conduitry @Rich-Harris |
@Rich-Harris In case this is useful, i built a css compiler stylis.js that does handle name-spacing keyframes and animations. |
I've been playing with this a little bit here. Not sure whether this is going to be a good direction to go in. In my branch, this particular issue is fixed, but there are a few not very nice things still. One is that PostCSS is introduced as an external (non-dev) dependency. It or one of its dependencies was depending on built-in Node modules, which the current build process doesn't seem to be able to handle. It's possible that this could be resolved with rollup-plugin-node-builtins but I didn't want to get bogged down in that for now. Using PostCSS to process the CSS did remove some of the ugly code from transform.js, but sadly the actual processing of a single selector remains the same, and ugly. I'm not very familiar with PostCSS at all, but I didn't immediately see a way of getting at something more processed than a single selector. |
Hm. Looks like this is not fixed yet with the new csstree-based parsing and transformation (REPL), but hopefully now this should be actually possible. I'll try to have a look at the new AST walker. edit: In fact, the workaround I suggested before (which was based on limitations of the old regexes) doesn't work anymore in 1.7+ either. The |
Using this walker in function walk ( node ) {
if ( node.type === 'Rule' ) {
transform( node );
} else if ( node.type === 'Atrule' && node.name === 'keyframes' ) {
// do not process keyframes declarations
} else if ( node.children ) {
node.children.forEach( walk );
} else if ( node.block ) {
walk( node.block );
} This doesn't take care of scoping the keyframes rules. Prefixing the |
Nice! Re scoping – one thing that occurs to me is that while it should be possible to transform keyframe declarations and <!-- Main.html -->
<Widget/>
<style>
div {
color: red;
}
@keyframes foo {...}
</style>
<!-- Widget.html -->
<div>this text is red, because child components inherit parent styles</div>
<style>
div {
/* foo would not be transformed correctly, so the animation would not
be inherited */
animation: foo 1s;
}
</style> So some CSS ( |
Something else I realized is that the keyframes names won't always show up in Doing two passes is probably necessary anyway - we don't want to be transforming keyframes names that aren't defined in that component, because that would break using keyframes that were defined globally. As for inheriting keyframes ... I don't see, currently, how to do that in a nice way. It seems like it would necessarily involve some runtime manipulation of the css, because at compile time a component has no idea what context it will be used in, or what the keyframes it's using have been renamed to. |
Yeah, I think you're right. Perhaps a workaround would be to have a way of marking keyframes as global, like this... @keyframes !foo {...} ...though it feels a bit hacky, and I suppose it can be worked around by just adding a regular stylesheet or |
Just pushed what I have now to https://github.com/Conduitry/svelte/tree/gh-245-keyframes-scoping It uses two passes - one where it collects the names of the keyframes definitions and transforms their definitions - and then a second where it does the normal This does allow you to use globally-defined keyframes within components, but will not allow you to globally define keyframes within components. I think that's reasonable behavior. |
@Conduitry You could also change the keyframe and animation names to |
@thysultan Hm I'm not sure quite what you're suggesting. What would the output be for the example CSS in the original post on this issue? |
.foo[svelte-3750005346], [svelte-3750005346] .foo {
animation: spin-360-3750005346 infinite 20s linear;
}
@keyframes spin-360-3750005346 {
from { transform: rotate(0deg); }
to { transform: rotate(360deg); }
} |
@thysultan As far as I can tell that's what I'm outputting now (apart from a Additionally, in the css AST, things like |
The
If the syntax for promoting a keyframe into the global scope is explicit it should be possible to identify i.e the
Come across this problem with stylis as well, since any variable number of identifiers can come with the short hand |
@Conduitry your branch looks good — could you open a PR please, and I'll merge it in? One small thing, it turns out keyframes are case-sensitive, so they should be left as-is and not lowercased. @thysultan afraid I can't quite understand what you're saying about allowing child components to inherit keyframes — how would that work, given that the hash is unique to each component? |
@Rich-Harris probably be a bit more tricky than i thought of it the first time round, maybe a parent reference escape hatch like |
Consider following example:
https://svelte.technology/repl/?version=1.6.3&gist=5390c2517520e606bb02ce8168f3b8b0
Currently, Svelte will output results as example:
Which animation won't works.
Expect that css should be:
The text was updated successfully, but these errors were encountered: