shannonmoeller / handlebars-layouts

Handlebars helpers which implement layout blocks similar to Jinja, Nunjucks (Swig), Pug (Jade), and Twig.
http://npm.im/handlebars-layouts
MIT License
361 stars 29 forks source link

Added {{#blockExists}} helper #17

Closed spacedawwwg closed 9 years ago

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-12.31%) to 87.69% when pulling 00b49aa1a4ef689abcf7e8b63f66aaa56a92b5b8 on spacedawwwg:master into b3d8c5d7018c4bf1d670bf75d494d09d95588bcc on shannonmoeller:master.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-1.52%) to 98.48% when pulling ed04fea5a16f06696049ad0953941a5c5847b2b7 on spacedawwwg:master into b3d8c5d7018c4bf1d670bf75d494d09d95588bcc on shannonmoeller:master.

spacedawwwg commented 9 years ago

@shannonmoeller what am I missing test wise? :-)

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-1.52%) to 98.48% when pulling 87224d81260fb836bc719d416cecb45726870598 on spacedawwwg:master into b3d8c5d7018c4bf1d670bf75d494d09d95588bcc on shannonmoeller:master.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 88a37e1e800a5ca0d7c0a588105a6b76065f298a on spacedawwwg:master into b3d8c5d7018c4bf1d670bf75d494d09d95588bcc on shannonmoeller:master.

shannonmoeller commented 9 years ago

This looks good. Thanks for meeting the code coverage and style requirements! Couple things:

  1. I don't think that blockExists is the right name. Technically, it should be contentExists.
  2. I'm conflicted about the "||" operator. Handlebars' core philosophy is "logicless templates" so introducing operators feels wrong. However, I immediately see the benefit.

Makes me wonder if it would be better to somehow expose the _layoutActions value. Perhaps using Handlebars.createFrame. That way people could bring their own comparison helpers to the party if they so desired:

{{#or @content.main @content.footer}}
    <div class="wrapper">
        {{#if @content.main}}
            <div class="main">
                {{#block "main"}}
            </div>
        {{/if}}
        {{#if @content.footer}}
            <div class="footer">
                {{#block "footer"}}
            </div>
        {{/if}}
    </div>
{{/or}}

Thoughts?

spacedawwwg commented 9 years ago

Absolutely. My suggestion/pull was a means-to-an-end for a project I am working on and simply wanted to share what I'd done.

I think what you have proposed above looks a lot nicer!

shannonmoeller commented 9 years ago

I've added the {{#if @content.main}} functionality to the develop branch. Mind kicking the tires?