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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply CSS scoping classes directly to AST (WIP) #1228

Merged
merged 4 commits into from
Mar 13, 2018
Merged

Conversation

Conduitry
Copy link
Member

(Based on the #1223 branch to also get its unit test.)

A couple of issues still remain here:

  • This breaks the unused-selector-ternary test because attributeMatches bails when attr.value.length > 1, which it now is, because there's the ternary part and the svelte-xyz class part. Not sure how to best handle this. Would it be practical to make the AST modifications after all the Selector#apply stuff?
  • The Element#addCssClass logic could use some more thought. I'm not sure how to handle when the class attribute's value is true (this would mean that there is a bare class attribute, what the heck do we do with that?); and TypeScript is also still complaining about classAttribute.value.length, not sure whether it needs appeasing there or what.

@Conduitry
Copy link
Member Author

Also it would be nice to make the CSS ref attribute into a class. That should theoretically not be too bad if we're modifying the AST instead of adding special cases to the rendering.

@Rich-Harris
Copy link
Member

Nice, this is definitely cleaner.

Would it be practical to make the AST modifications after all the Selector#apply stuff?

Maybe a good way to do this would for the Stylesheet to have a Set of all the nodes that need the class, so generator.stylesheet.apply(node) may or may not add node to that set. Then, after the walk call in generator.walkTemplate, we could have something like generator.stylesheet.reify (or whatever) that does the actual mutation by iterating over that set.

That would eliminate the need for this. _addedCssClass as well.

I think we play a bit fast and loose with bare attributes. For string attributes (as opposed to boolean attributes), it means the empty string rather than true. And we know class is a string attribute. So in effect the current behaviour of this PR is correct, I think.

Agree re ref attribute, I reckon that'd be better.

@Conduitry
Copy link
Member Author

Conduitry commented Mar 12, 2018

I like the idea of the Set and of Stylesheet#reify ... However I'm having some trouble figuring out exactly where we should call that from. Obviously it needs to be after the traversing the Nodes and styles to find out which elements need the class added to them but before we actually generate the javascript for the Nodes. And we need to worry about DOM and SSR generation. I have some big-picture stuff missing here.

Edit: Whoops, you said in your comment already where you think it should go. Reading can save time ...

Edit again: Right after generator.walkTemplate() seems like it might be too early, as that's still failing the unused-ternary-selector test. I'll push what I have so far.

Edit again again: Okay it looks like putting this.stylesheet.reify(); right after this.walkTemplate(); works for SSR mode but not for DOM mode.

@Rich-Harris
Copy link
Member

figured it out — stylesheet.apply was being called twice, once inside walkTemplate and once inside Element#init. In between, the AST was modified. will push up a fix

@Rich-Harris
Copy link
Member

ok, looks like we're set. will merge this to close #1223, and open a separate issue for refs-as-classes

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