pugjs / pug

Pug – robust, elegant, feature rich template engine for Node.js
https://pugjs.org
21.68k stars 1.95k forks source link

Allow multiple blocks to be defined inside mixins and includes #631

Closed jasonkuhrt closed 10 years ago

jasonkuhrt commented 12 years ago

I would like to be able to write my markup like thus:

//panel.jade

section.panel
  block head
    header
      h1.title!= title
  .content
    yield

invocation:

//some_file.jade
    include panels/panel
      block head
        //nothing
      img( src="#{work.urls.main}/poster_280.jpg" )

Update based on feedback the new proposal would be:

//panel.jade
mixin panel
  section.panel
    block head
      header
        h1.title Default
    .content
      yield

invocation:

//some_file.jade
    + panels/panel
      disable head
      img( src="#{work.urls.main}/poster_280.jpg" )
chowey commented 12 years ago

This sort of makes extend and include behave the same. Is that a good direction to take?

jasonkuhrt commented 12 years ago

Hey @chowey

chowey commented 12 years ago

Yeah actually mixins kind of work like extends right now, except they take arguments, are defined in the same file, and don't have the "append" or "prepend" features.

We could make mixins do append and prepend (actually a more consistent API, don't know why I didn't think of that earlier).

I think if we go that route, then include should just include the file, extends is redundant because include/mixins could do the work that extends did before.

So basically I propose that you don't do

include panels/panel
  block head
    //nothing
  img( src="#{work.urls.main}/poster_280.jpg" )

but instead something like

include panels/panel

+panel
  block head
    //nothing
  img( src="#{work.urls.main}/poster_280.jpg" )

This would make panel defined as a mixin instead. Then, in mixins, we would support yield for non-block statements, and blocks would be supported as they are in extends.

It is not backwards compatible in mixins, but that may be ok because the new mixin block features are not documented yet. I think the existing include/extends behavior would need to be left as-is for backwards compatibility.

chowey commented 12 years ago

(For the above, you would need to define mixin panel in the "panels/panel.jade" file.)

donpark commented 12 years ago

I think overall architecture needs reviewing, particularly with seamless WebComponents support in mind, instead of just looking at include, extends, block, and mixin.

Also we need to keep in mind that there are two different units of work here: component/function and module/file.

chowey commented 12 years ago

The purpose of extends is to provide a layout. It does that nicely. It is not meant to build widgets.

The purpose of include is to provide drop-in-place markup. You can do some fancy things with includes by using loops and including multiple times, changing variables with each loop. But it is pretty much limited to that.

The purpose of mixin is to give a shortcut for typing in markup that you will be re-using. It takes arguments. You can pass blocks to it (a new feature). It sort of works to build widgets but you don't get multiple blocks.

None of these is fully a "template".

I maintain that the best candidate for templating is mixins. I say this because the purpose of mixins is closest to that of templates. So we would be fixing up mixins to better achieve their design intent, not changing the design intent entirely.

We would need to improve mixins like so:

mixin panel
  .panel
    .header
      block header
        h1 Default title
    yield
    if block.footer
      .footer
        block footer

+panel
  block header
    h1 My important announcement
  block footer
    a(href='next.html') Next
  p This is my main content.
  p And this is another important point.

which would render:

<div class="panel">
  <div class="header">
    <h1>My important announcement</h1>
  </div>
  <p>This is my main content.</p>
  <p>And this is another important point.</p>
  <div class="footer"><a href="next.html">Next</a></div>
</div>

If you want to put your mixin definitions in an external file (e.g. you plan to re-use them), then you would include that file.

Is that more or less what we're asking for?

It is also totally possible to make include work like you say. However, I think mixins were intended to be the templating feature.

I could work on implementing this but I won't start unless @visionmedia says ok. It is a fair bit of work to do if it won't be approved anyway.

jasonkuhrt commented 12 years ago

@chowey I think you're interpretation would certainly be a big improvement.

If we create a way for mixins handle files like include then we can also deprecate include.

Playing around:

mixin panel
  .panel
    .header
      block header
        h1 Default title
    yield
    if block.footer
      .footer
        block footer
+ panels/panel
+ panel
  block header
    h1 My important announcement
  block footer
    a(href='next.html') Next
  p This is my main content.
  p And this is another important point.

I also then see an opportunity for direct mixin access without needing to include, like this:

+ panels/panel
  block header
    h1 My important announcement
  block footer
    a(href='next.html') Next
  p This is my main content.
  p And this is another important point.

To encourage code-sharing / mixing-libraries we could facilitate namespaces like:

soundtrack-panel = +panels/soundtrack
+ soundtrack-panel
  block header
    h1 My important announcement
  block footer
    a(href='next.html') Next
  p This is my main content.
  p And this is another important point.

or hash-access like:

+ panels
// or panels = +panels
+ panels.big
  block header
    h1 My important announcement
  block footer
    a(href='next.html') Next
  p This is my main content.
  p And this is another important point.

We could say that a mixin without params or nested content works like an include. That seems intuitive?

jasonkuhrt commented 12 years ago

@donpark Details would be great. Sounds interesting...

chowey commented 12 years ago

I am against making mixins do work with files. I think importing a file and defining a mixin should be logically separate.

You can do

include lib/mytemplates
+panel
  // ...

instead of

+lib/mytemplates/panel
  // ...

and I think keeping it separate is logically better. It is also less messy on the implementation side.

The + syntax for calling a mixin has been around since 0.24 or something (sort of in its alpha stage).

jasonkuhrt commented 12 years ago

@chowey I might have a different perspective but I see yours very clearly and agree it makes sense. The existence of include and mixin is not a problem and as you say there is a perfectly reasonable case why its more logical this way.

tj commented 12 years ago

things have definitely evolved a bit but I still think they're pretty distinct sets of functionality other than maybe the new include block stuff. include could potentially replace extends if we had multiple block support


include layout
  append head
    script(src='jquery.js')

  block content
    h2 stuff here

or something, not in love with it but I definitely prefer less concepts as long as they work nicely. Personally I dont think building widgets at the template engine level is all that great, a lot of that can be done in client-side js and works better IMO, at least for my use-cases, I pretty much never build widgets with Jade that inherit from each other etc, and only mixins for really repetitive things that can't really be expressed clearly without a bit of abstraction

jasonkuhrt commented 12 years ago

@visionmedia at-scale I see your point about js widgets but for a lot of brochure sites, micro blogs, or portfolio sites I think Jade should still offer a great code-reuse experience. And its not as if we have to sacrifice simplicity to achieve it. Merely adding multiple block support to mixins will get us a lot of the way there.

tj commented 12 years ago

definitely

donpark commented 12 years ago

If deprecating extends and adding general block support is what the consensus is, I like it.

jasonkuhrt commented 12 years ago

Can we make a decision so that @chowey might spike an implementation? It seems like we have a pretty general agreement about how to improve the system.

tj commented 12 years ago

personally I have no issue with what we currently have but I'm open to the suggestions if it works out fine

jasonkuhrt commented 12 years ago

There's one nuance not covered yet that replacing extends with include introduces:

With extends:

extends foo

block a
  ...

block b
  ...

content...

Now with include:

include foo
  block a
    ...
  block b
    ...
  content...

// or

include foo
  content...

// access block 'a' defined within foo without nesting?
block a
  ...

// or namespace access block 'a' defined within foo without nesting?
block foo.a
  ...

// so that if we have another include
include bar

we can extend another block of the same name ('a') defined inside bar, again without nesting

block bar.a
  ...

Its more complicated given that we can have many includes whereas extend was simple because we were limited to one.

tj commented 12 years ago

yeah, that's why im not personally too worried on changing this immediately until we have a good solution

jasonkuhrt commented 12 years ago

@visionmedia If we reduced the commit to just improving mixin block abilities, I think that would be a good safe bet without risking future backtracking.

donpark commented 12 years ago

Just brainstorming. What if extends is re-purposed as a way to redefine/override include operations like this?

extends lib/foo
  block a
    ...

extends lib/bar
  block b
    ...

.container
  .foo
    include lib/foo
  .bar
    include lib/bar
chowey commented 12 years ago

I made a patch that makes include work with blocks. It turns out this is almost trivial to implement.

On the other hand, making mixins work like I said is horrible.

So include support is what we get.

jasonkuhrt commented 12 years ago

@chowey Thanks man! It's a nice addition at least.

donpark commented 12 years ago

thanks @chowey. this will come in handy.

holic commented 12 years ago

Any updates on this?

darky commented 11 years ago

Yeah, it's not release...

maxnordlund commented 11 years ago

I bumped into this problem today to my surprise. I had hoped you could write widgets, jade files that includes all the CSS/JS and HTML for it to work. The snag was that you can't use blocks in included files, as discussed above.

Layout.jade

!!! 5
html
  head
    block styles
      link(rel="stylesheet", href="style.css")
    block scripts
      script(src="jquery.js")
  body
    block content

Widget.jade

append styles
  link(rel="stylesheet", href="widget.css")
append scripts
  script(src="widget.js")

p Some fancy widget
canvas ....

These are tied by another jade file, like Index.jade

block content
  header
    ...
  #content
  #widgets
    include widget

But instead of appending the link and script tags in the correct place they are at best dumped straight into the body if you add a block content somewhere.

This is how I imagined how extend and include where suposed to work. I hope this pr doesn't get lost and that @visionmedia tries to take all these feedback into consideration, which from what I get see here doesn't feel like it.

tj commented 11 years ago

https://github.com/component/component is a lot better for that, it's a bit of a hacky thing to attempt on the server end IMO, but i can see how it's desirable for people who have to use server rendering.

igor10k commented 10 years ago

Twig implements something similar to what is being described here as an embed

I'm personally quite surprised that this feature is not often requested. Creating reusable pieces of code is always great and this feature really helps. I see people ending up with copy-pasting the same html pieces over and over and using includes.

And using just one block within a mixin is kind of not an option because you can't be sure if tomorrow you won't need another block there. And if you will you'll have to reimagine the mixin completely.

itsravenous commented 10 years ago

I would second all the comments here about this being a great boon for code re-use. I'm currently using a 'component factory' approach to many projects, and this would make things a lot easier (this, or solving #374).

ForbesLindesay commented 10 years ago

This has seen relatively little activity for a long time. If someone wants do implement it (along with tests) I'm happy to consider it, but I'm not going to implement it myself.

bigpopakap commented 10 years ago

This would be extremely useful in my current project. This is an old issue so things might have changed by now, but I can't find any doc on it. So my question is: is there now a way to achieve using multiple named blocks in a mixin?

@visionmedia

igor10k commented 10 years ago

Nothing was changed regarding to this case. But there's a hacky way to implement something similar though by splitting mixin code into few nested mixins, like

mixin main
  mixin header
    // some code that should always be there
  mixin footer
    // some more code
  block

Then you'll be able to insert code between those nested mixins.

+main
  p Welcome to my website
  +header
  p Some great content goes here
  +footer
  p Copyright 2014

This workaround is not as convenient as named blocks would be but it saved me once. :)

ForbesLindesay commented 10 years ago

@Igor10k that does work really well. I've actually been using similar techniques in some of my projects for a while now. It would be really great to add some jade test cases that demonstrate the feature, to ensure we don't accidentally break that use case in the future.

bigpopakap commented 10 years ago

Thanks @Igor10k! I got it to work, but with a slightly different syntax than you provided. My project (which you can see linked above since I accidentally added a reference to this issue) uses Jade version 1.3.1

The syntax I'm using now is as follows:

//- mixin declaration
mixin imageAndCaption()
    div.someClass
        +image()
        div.myCaption
            +caption()

//- use the mixin
+imageAndCaption()
    mixin image
        img(src="/my/image.png")
    mixin caption
        p this is my caption

So I never had to use the block keyword. And the nested mixins are defined by the caller of the enclosing mixin, not defined in the enclosing mixin itself

This syntax makes a lot of sense and I really like it. It also allows variables to be passed around, which makes iterations easy (which is what I actually have in my project)

itsravenous commented 10 years ago

@Igor10k @bigpopakap that looks like a nice way of doing it - cheers for sharing. @bigpopakap I especially like your take on it. Because I work in a very component-based manner, one thing I would add is namespacing the nested mixins to their parent. So:

+post("Post title")
    mixin post_teaser
        p [...]
    mixin post_body
        p [...]
        p [...]
        p [...]
ForbesLindesay commented 10 years ago

@bigpopakap That code example doesn't (and shouldn't) work. The issue is that the mixins you are writing inside the caller's block won't ever actually get defined. The JavaScript generated by that example is:

    var buf = [];
    var jade_mixins = {};
    jade_mixins["imageAndCaption"] = function() {
        var block = this && this.block, attributes = this && this.attributes || {};
        buf.push('<div class="someClass">');
        jade_mixins["image"]();
        buf.push('<div class="myCaption">');
        jade_mixins["caption"]();
        buf.push("</div></div>");
    };
    jade_mixins["imageAndCaption"].call({
        block: function() {
            jade_mixins["image"] = function() {
                var block = this && this.block, attributes = this && this.attributes || {};
                buf.push('<img src="/my/image.png"/>');
            };
            jade_mixins["caption"] = function() {
                var block = this && this.block, attributes = this && this.attributes || {};
                buf.push("<p>this is my caption</p>");
            };
        }
    });
    return buf.join("");

As you can see, the calls to jade_mixins["image"] = and jade_mixins["caption"] = never actually get called since they are inside the block function. To fix this, we have to actually add the block keyword at the start of the imageAndCaption mixin. It is there just to cause the inner mixins to get defined:

//- mixin declaration
mixin imageAndCaption()
    block
    div.someClass
        +image()
        div.myCaption
            +caption()

//- use the mixin
+imageAndCaption()
    mixin image
        img(src="/my/image.png")
    mixin caption
        p this is my caption

which results in JavaScript like:

    var buf = [];
    var jade_mixins = {};
    jade_mixins["imageAndCaption"] = function() {
        var block = this && this.block, attributes = this && this.attributes || {};
        block && block();
        buf.push('<div class="someClass">');
        jade_mixins["image"]();
        buf.push('<div class="myCaption">');
        jade_mixins["caption"]();
        buf.push("</div></div>");
    };
    jade_mixins["imageAndCaption"].call({
        block: function() {
            jade_mixins["image"] = function() {
                var block = this && this.block, attributes = this && this.attributes || {};
                buf.push('<img src="/my/image.png"/>');
            };
            jade_mixins["caption"] = function() {
                var block = this && this.block, attributes = this && this.attributes || {};
                buf.push("<p>this is my caption</p>");
            };
        }
    });
    return buf.join("");

In turn resulting in the html:

<div class="someClass">
  <img src="/my/image.png"/>
  <div class="myCaption">
    <p>this is my caption</p>
  </div>
</div>

You also need to be careful to always define both inner mixins or the previous definition may get used. An alternative would be to define defaults like:

//- mixin declaration
mixin imageAndCaption()
    mixin image
      //- this will get overridden
      img(src="/my/placeholder.png")
    mixin caption
      //- this will get overridden
      p Un-captioned image
    block
    div.someClass
        +image()
        div.myCaption
            +caption()

//- use the mixin
+imageAndCaption()
    mixin image
        img(src="/my/image.png")
    mixin caption
        p this is my caption
bigpopakap commented 10 years ago

Hmm I'm not sure what's happening then, because I have it working. Could it be that in my project, mixin image and mixin caption are actually being defined globally?

If you have the time, my relevant change is linked below. In that code, carousel is the equivalent of imageAndCaption from my example above, carouselImage is the equivalent of image, and carouselCaption is the equivalent of caption

https://github.com/bigpopakap/kapileaswar/commit/ccec5d735e9e12f434354b829172fdc28df24d8a

thomporter commented 10 years ago

@ForbesLindesay Attempting your alternative to define defaults is getting me "Anonymous blocks are not allowed unless they are part of a mixin." I tested you code as well as the hacks I made to mine. =)

Would really like to be able to setup the defaults, as 90% of the calls would use it...

I'm on jade 1.3.1

ForbesLindesay commented 10 years ago

Sorry, that's a bug. I've opened an issue to get it fixed.

anticamper commented 10 years ago

@thomporter this worked for me:

file mixins/box.jade:
mixin box   
    //- MIXIN BLOCK
    block

    //- WORKAROUND FOR DEFAULTS
    mixin mixin(mixin,mixinDefault)
        - try {
            if mixin
                +#{mixin}
            else
                - throw new Error();
        - } catch(err) {
            if mixinDefault
                +#{mixinDefault}
        - }

    //- DEFAULT COMPONENTS
    mixin headerDefault
        p
            | i am a default header
    mixin contentDefault
        p
            | i am default content
    mixin footerDefault
        p
            | i am a default footer 

    //- BOX LAYOUT
    div&attributes(attributes)
        +mixin('header','headerDefault')
        +mixin('content','contentDefault')
        +mixin('footer','footerDefault')
file layout.jade:
//- [...]
include mixins/box
+box
    mixin header
        p
            | i am a overridden header
klarezz commented 9 years ago

another suggestion for named blocks:

// initialization
- var globals={};

mixin set(key)
    - globals[key]=this.block

mixin A
    block
    - globals.content()

+A
    +set('content')
        p A content

is there a way to pass a jade syntax to javascript without using a mixin.block?

TimothyGu commented 9 years ago

Condensed version of @anticamper's workaround:

mixin myMixinWithTwoBlocks(uniqueID)
  block

  mixin contentDefault
    p No content provided

  .author
    +author
  .body
    +content
+myMixinWithTwoBlocks
  mixin author
    a(href="https://my.website") Timothy
  mixin content
    p Hola señor. ¿Cómo está?
    p Bien, ¡gracias!

+myMixinWithTwoBlocks
  mixin author
    | Juan
  mixin content
    +contentDefault

The default trick doesn't work, as the lifetime of the first content mixin lasts throughout the file. As a result, you have to always manually specify content to be contentDefault.

Output (manually beautified):

<div class="author">
  <a href="https://my.website">Timothy</a>
</div>
<div class="body">
  <p>Hola señor. ¿Cómo está?</p>
  <p>Bien, ¡gracias!</p>
</div>
<div class="author">
  Juan
</div>
<div class="body">
  <p>No content provided</p>
</div>
CREEATION commented 9 years ago

Thanks @anticamper & @TimothyGu - pure awesomeness. I just built a tabs-element mixin with this, works like a charm! <3

isaactzab commented 8 years ago

Thanks @anticamper, @TimothyGu , and @CREEATION Nested mixins are good solution, but what if I what repeat a block code or change the block orders within my mixin?

mixin table
    block tableheader
    block customrow
    - var row = 0
    while row<10
        - var col = 0
        tr
            while col<10
                td cell
                col++
        row++
    block customrow
    block tablefooter
+table
    block tableheader
        //- table header here
    block tablefooter
        //- table footer here
    block customrow
        //- custom row here
nataliawww commented 7 years ago

Found this solution without having to use nested mixins https://codepen.io/nekitk/pen/bdMZgb?editors=1010

Ad-Ok commented 7 years ago

@nataliawojtkowska that's great, but what if I need conditions for displaying inner content? For example, show blocks.main() only?

nataliawww commented 7 years ago

@Ad-Ok Then you can add an if statement like

if blocks.side
    - blocks.side()

https://codepen.io/14islands/pen/QqXYrb?editors=1010

If that's what you mean 😊

EDIT: @Ad-Ok I forgot to clear the blocks array, otherwise you'd still get the blocks.side from setting it previously. Take a look at the updated pen

Ad-Ok commented 7 years ago

@nataliawojtkowska Thank you very much! It's exactly what I wanted.

vectorjon commented 3 years ago

It doesn't look like multiple blocks in mixins are implemented yet. I've looked around and saw some workarounds and decided to try and improve their functionality. Here is what I came up with: https://codepen.io/Vectorjon/pen/GRmYWjN

mixin block(key, marker=false)
    - global.my_blocks = global.my_blocks || {}
    if marker
        if global.my_blocks[key]
            - let temp = {}
            - temp[key] = global.my_blocks[key]
            - global.my_blocks[key] = null
            - temp[key]()
        else
            block
    else
        - global.my_blocks[key] = this.block

Quick overview on usage:

mixin example
    block
    .part-1
        +block('one', true)
            p one's default
    .part-2
        +block('two', true)

+example
    +block('one')
        p whatever
        p you
        p want
    +block('two')
        p also
        p whatever

Hope you find it useful. Comment on my CodePen if you find any bugs or have suggestions.