-
Notifications
You must be signed in to change notification settings - Fork 27
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 test cases for spread attributes #52
base: master
Are you sure you want to change the base?
Conversation
This only works if it's the first attribute in the list, and doesn't work for more complex JavaScript expressions. It would be nice to adapt it to work more reliably. |
Yeah, since the dots might be part of a JavaScript expression (e.g. |
7708e23
to
59f392c
Compare
Just implemented that. All tests pass. |
@@ -0,0 +1,5 @@ | |||
div(...attrs) | |||
div(...attrs=foo) |
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.
Just a question: what is the foo
? Why couldn't one just do (...foo)
?
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.
My thinking was that it could mean you can namespace things: e.g.
div(...data=myDataAttributes)
compiled with:
{myDataAttributes: {foo: 'foo-val', bar: 'bar-val'}}
results in:
<div data-foo="foo-val" data-bar="bar-val"></div>
Probably this is the wrong syntax for that though. I think I would prefer to support something like:
div(data=...myDataAttributes)
or
div(data...=myDataAttributes)
as I think that reads better/more clearly. Probably best to remove support for having a "value" for spread attributes. If we make sure it's a syntax-error for now, we can always add it in later.
Awesome. One last thing before we merge this: We should define a new token-type of |
So basically there are two classes of spread attributes:
I'll use |
Sounds good. I'm inclined to only implement |
I'm labeling this as a breaking change because we currently allow attributes at the start to be like |
Yep, sounds about right. |
Fixes column tracking of quoted attribute keys with commas. Fixes `a(attr, 'weird=')`.
ebe6467
to
6d406e4
Compare
Pushed a few commits that 1. added an unescaped version ( |
These currently already lex without any problems. I’d like to use these to replace
&attributes
and provide proper escaping in the process