Make mixin blocks act like blocks #600

Closed
wants to merge 9 commits into
from

Projects

None yet

3 participants

@chowey
chowey commented Apr 22, 2012

This removes the contents keyword, and adds the block keyword:

  • contents was a string, is now removed
  • block is a function, is called whenever the block statement is used within a mixin
  • Because block is a function, it is called "in-context" and can therefore render prettily
  • block can be tested for truthy-ness, e.g. if (block) works as expected
chowey added some commits Apr 22, 2012
@chowey chowey Make mixin blocks act like blocks
This removes the `contents` keyword, and adds the `block` keyword:

* `contents` was a string, is now removed

* `block` is a function, is called whenever the `block` statement is used within a mixin

* Because `block` is a function, it is called "in-context" and can therefore render prettily
cf886ec
@chowey chowey Update help file eec0d00
@chowey
chowey commented Apr 22, 2012

I hijacked the block keyword for mixins. This may not be the best keyword?

I did more thorough testing for this pull request. I tried many combinations of indents and mixins, and this correctly renders them with {pretty: true}.

I don't like how block is a function and can be accessed directly by the user as a variable. This makes the following work, because block.toString() is implicitly called, but it sure looks ugly:

mixin foo
  p= block
@kizu
kizu commented Apr 23, 2012

New syntax of mixins/blocks is great!

However, I stumbled with this case: I want a mixin that can have arguments, but can be without them. And if I do something like that:

mixin foo(bar)
  h2 the block
  p
    if block
      block
    else
      | no block

  h2 the bar
  p
    if bar
      =bar
    else
      | no bar

h1 Mixin with an argument
+foo("hey!")
  | I'm a content!

h1 Mixin without an argument

+foo
  | I'm a content!

Then if I call the mixin with an argument it's all ok, but if I'll call the mixin without an argument, I'll get some strangeness:

<h1>Mixin without an argument</h1>
<h2>the block</h2>
<p>no block
</p>
<h2>the bar</h2>
<p>function (){
__jade.unshift({ lineno: 19, filename: __jade[0].filename });
__jade.unshift({ lineno: 19, filename: __jade[0].filename });
buf.push('I\'m a content!');
__jade.shift();
__jade.shift();
}
</p>

There would be no block and the bar would become the content.

@kizu
kizu commented Apr 23, 2012

Also! How hard it would be to make Mixins work like normal tags? So we could do

+foo Some content

or

+foo.
  Some content

That would be awesome!

@chowey
chowey commented Apr 23, 2012

Mixins without the parentheses is easy to do, so long as you are still passing arguments like function arguments. So this could be made to work:

mixin foo(bar, baz)
  p= bar
  p= baz

+foo "Some content", "Some other content"

I guess you want it so that if a mixin is missing parentheses after it, then it should act just like one big text string? And this would be the first arg?

mixin center(stuff)
  div.centered= stuff

+center Some content

This change would be beyond the scope of this commit.

chowey added some commits Apr 23, 2012
@chowey chowey Remove `mixing` arg which is already covered by `parentIndents` 8ea4cd0
@chowey chowey Pass mixin blocks as `this`
By using `Function.call`, we can pass mixin blocks without affecting arguments, which otherwise leads to weird bugs if optional arguments are being used in the mixin.

This also makes the `block` variable true/null instead of returning the function itself, which is safer for users.  The function is instead available as `this.block`.
3f7b2a7
@chowey
chowey commented Apr 23, 2012

Okay, this should fix your first bug, anyway. Let me know if it works.

The second point should be its separate issue since it won't be part of this commit. (This commit isn't even accepted yet.)

@kizu
kizu commented Apr 23, 2012

Yep, the fix works, thanks!

The second part is a bit different — I think that the part after the space must be the just like the text block, not like the argument.

It would be supercool if the mixins could work just like a normal tag, so this:

+foo bar

would be equal to

+foo
  | bar

And in more complex cases it could still behave like the simple tag: so the following would be possible:

+foo= someVariable

+foo: .bar: +foo("baz")

etc. However, I agree that it isn't in this scope, so I'll fill it later as a separate issue.


Thanks again, the mixins in Jade are becoming better and better, I really looking forward to using them :)

@chowey
chowey commented Apr 23, 2012

@visionmedia could you review? There seems to be no performance impact. It passes all tests.

I think using mixin.call(contextObject, ...) instead of mixin(...) opens up some possibilities, since there can be extra jade-only goodies provided for mixins that would not be available for normal functions.

chowey added some commits Apr 23, 2012
@chowey chowey Update test case
(The "block" property automatically propogates up the chain through mixins.)
5dd513a
@chowey chowey Removed code that no longer is needed 4c1a9a8
@chowey chowey Add support for mixins "working like normal tags"
Any text after the parentheses acts like a text node.

It can be accessed with the `block` keyword.
22f37d6
@chowey
chowey commented Apr 24, 2012

@kizu, I was able to add the "work like normal tags" style of mixin.

How it works is like so:

mixin foo(bar)
  - if (!bar) bar = 'centered'
  div(class=bar)
    block

body
  +foo Hello
  +foo("left-align") World!
  +foo
    p Some content

which renders like

<body>
  <div class="centered">Hello
  </div>
  <div class="left-align">World!
  </div>
  <div class="centered">
    <p>Some content</p>
  </div>
</body>

So the text you put after the mixin, after the parentheses (which are optional), act like a text node which goes where your block statement goes.

Also, any additional block statement just gets appended.

This is the most intuitive and straightforward way I could think to implement this.

It doesn't support the class and id syntax for normal tags, but it is a start.

@kizu
kizu commented Apr 24, 2012

@chowey Wow, cool!

BTW, what if there'd be an arguments variable like in Stylus?

So you could just do

mixin foo
  div(arguments)
    block

+foo(class="baz")
  p Some content

And if you'd support the class and id as it works now for tags, it would become even this:

mixin foo
  div(arguments)
    block

+foo.baz
  p Some content

Also, if there would be a way to access some named/special arguments, you could do this:

mixin item
  li.menu-item(class=arguments.class)
    a(href=arguments.href)
      block

ul.menu
  +item.current(href="/foo/") Foo 
  +item(href="/bar/") Bar
  +item(href="/baz/") Baz

…And many more powerful mixins like this.

@chowey
chowey commented Apr 24, 2012

The problem with attributes is that the syntax is a bit ambiguous, because parentheses are also used for passing arguments.

I guess we could require explicit = in attribute declarations. So you could do foo(checked=true) for an attribute and foo(true) for an argument.

I would not use the arguments variable like in Stylus, because arguments is already a javascript reserved word and someone may want to access it. I would use attributes or attrs something.

@chowey chowey Make `mixin` parse like `tag`
(Arguments for the mixin MUST come first)
41f495f
@kizu
kizu commented Apr 25, 2012

The exact syntax doesn't matter, so attributes or attrs would be great to.

@chowey
chowey commented Apr 25, 2012

Okay, here is preliminary support for attributes. You must pass arguments to the mixin first, then attributes after that. Otherwise it should work just like a tag.

+foo(arg1, arg2)#id.class1.class2(href="www.foo.com") Text
  p More content

They are accessed with attributes.id, attributes.href, etc.

There is no support for div(attributes) within the mixin doing anything meaningful.

@tj
Collaborator
tj commented Apr 25, 2012

shit, I have 150+ notifications I didn't see this one and I implemented block this morning

@tj
Collaborator
tj commented Apr 25, 2012

as for classes/ids im not sure how I feel about those with mixins

@kizu
kizu commented Apr 25, 2012

@chowey Wow, so awesome already! Some minor issues:

If you have a mixin like this:

mixin bar
  .bar(class=attributes.class)
    block

+bar baz

then you'd have this render:

<div class="bar undefined">baz
</div>

So if the mixin have the some class and have the attributes.class, that is not specified on the mixin call, it would render undefined class.

Also, is there a way to throw all the attributes at once? Like

mixin bar
  .bar(attributes)
    block

so all the passed attributes would go to defined block. It can be useful if I would like to create a mixin that would add some wrappers to block, so if I need some extras in every link, I could do

mixin link
  span.link-wrapper
    a.link(attributes)
      span.link-inner

+link.current no link here
+link(href="#baz") baz
+link.external(href="#bar") bar

And that would give me the desired code. Now I need to write all the attributes manually here like

a.link(class=attributes.class, href=attributes.href, target=attributes.target)

@visionmedia classes&ids, as attributes, can be really useful in mixins when using them for blocks/objects in smacss/bem-like markup. So you could create a mixin for some block and then easily adjust it based on classes, easily add id's or anything to it. It's just a shortcut, but can be very handy.

Also, look at the examples that I provided earlier — all of them would be much uglier if we'd make them without such syntax, all of those things would go in arguments etc. If you need more examples how this can be useful — just tell me and I'll provide them :)

@chowey
chowey commented Apr 25, 2012

Okay @visionmedia you want to ditch this change, or what?

I was actually just trying to get mixin blocks to print pretty. That's really all I was doing with this pull request to start with. In order to do that, you need the function-style of block like I did. The simple string version will always print with no indents.

I can roll this back so that there is no attribute support. Or split that to a different pull request. But I do think you should consider using the function-style of block.

Give me some guidance and I will help!

@tj
Collaborator
tj commented Apr 25, 2012

we should definitely separate things into different PRs, we can do block == != block() with that thing I pushed, I forgot we were going to defer for pretty-printing

@tj tj closed this Apr 25, 2012
This was referenced Apr 25, 2012
@tj
Collaborator
tj commented Apr 26, 2012

I like it in some instances but I also don't want to reinvent regular javascript arguments for mixins in some sense, especially since this adds a reasonable amount of complexity that needs to be maintained. I think we need to visit some concrete use-cases. I like the look of it for things like +item(href='foo'), and things like .form(method='get', action='foo'). One thing I really don't like is trying to disambiguate between "regular" js args and this form of args. I'll definitely have to think on this one for a bit and maybe try out some examples

@chowey
chowey commented Apr 26, 2012

Fair enough.

My thought was that we want to disambiguate between js args and mixin-specific keywords. They should be conceptually separate. To the user the mixin-specific keywords should just work, without him having to worry about the javascript behind it.

The real reason I did it though is because I couldn't see a better way to handle a use case like this:

mixin foo(bar)
  - if (bar)
    h1= bar
  block

+foo("Title")
  p Some content
+foo
  p Some more content

In this example, when block is passed as an argument, if (bar) will always be truthy since bar will get the block variable if bar is omitted from the mixin call. Further, block becomes unavailable.

@tj
Collaborator
tj commented Apr 26, 2012

wouldn't your .call() change fix that? I'm just talking attributes for mixins

@chowey
chowey commented Apr 26, 2012

Oh, yes.

Currently my test is this regex in the Lexer: /^ *[-\w]+ *=/. It tests this against whatever is in the first pair of parentheses. If this regex tests false, then it assumes you are passing arguments.

This should only fail if you try to use attributes like +inputBox(checked) which would assume that checked is an argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment