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

Preserve/inherit indentation levels. #237

Closed

Conversation

brianreavis
Copy link

This fix is purely aesthetic, but I think it's pretty important (especially at DIY where we hope to have kids reading the page source). Currently with handlebars, using partials causes indentation levels of the output to get borked. Here's the scenario:

<html>
    <head>
        {{> head}}
    </head>
</html>

"head" partial:

<title>{{title}}</title>
<style type="text/css">
    body { background: #fff; }
</style>

Current Output:

<html>
    <head>
        <title>{{title}}</title>
<style type="text/css">
    body { background: #fff; }
</style>
    </head>
</html>

Fixed Output:

With this update, the output looks like this:

<html>
    <head>
        <title>{{title}}</title>
        <style type="text/css">
            body { background: #fff; }
        </style>
    </head>
</html>

The update reads the indentation level of the last line of the content block preceding the partial, and transfers it to every line of the partial output (except the first). Works with spaces, tabs, or a mixture of the two.

Note: I made sure that the modifications to runtime.js will not break existing precompiled templates.

(p.s. Sorry for submitting this twice... I forgot about another change I wanted to submit as a separate pull request, so I had to set up some branches.)

@wagenet
Copy link
Collaborator

wagenet commented May 25, 2012

@wycats, what are your thoughts on this?

@brianreavis
Copy link
Author

Update: I was realizing yesterday that partials are only half the battle. This has the same behavior too:

<html>
    <body>
        {{{body}}}
    </body>
</html>

With that said, whatever the stance is, let's not merge this in until I can get this part fixed too. And with that addition, I want to make the indentation behavior completely optional (via something like Handlebars.indent = true;).

@brianreavis
Copy link
Author

@wagenet @wycats Mind taking another look? I made some structural changes and made the indentation completely optional via the Handlebars.indent option (which is false by default). It also now works with mustache blocks.

To enable indentation:

Handlebars.indent = true;

@wagenet
Copy link
Collaborator

wagenet commented Jul 31, 2012

@brianreavis Sorry we've been so slow in reviewing this. Is there any chance you could rebase to get this to merge cleanly? Maybe also squash down some of the commits while you're at it.

@wagenet
Copy link
Collaborator

wagenet commented Jul 31, 2012

Also, is it possible for you to write tests for this?

@wycats
Copy link
Collaborator

wycats commented Oct 13, 2012

My main concern about stuff like this (similar to #336) is that it introduces seemingly-magic behavior in order to achieve whitespace-related behavior that particular use-cases consider appropriate. On the flip-side, in whitespace-sensitive situations (pre, textarea), I could imagine this behavior being extremely frustrating.

Simply making it an option doesn't really help the situation, since most people will see the problematic behavior, get frustrated by the situation, and never find the option. Making it a flag you need to turn on manually will help the people who submitted the ticket, but again, very few people who need the behavior will find the flag.

Because of these opposing constraints, my inclination is to have the output consistently do what it says, and not mess around with whitespace. I could probably be persuaded to support a flag, if the pull request came with a doc-patch to handlebars-site and tests.

For now, I'm closing this. Please reopen if you have a more complete patch. Thanks!

@wycats wycats closed this Oct 13, 2012
@MarcDiethelm
Copy link

I would very much like to see this in a future version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants