search

cloudfour / drizzle

A streamlined tool for developing, documenting and presenting UI pattern libraries.
https://cloudfour.com/thinks/introducing-drizzle/
MIT License
666 stars 32 forks source link

`blank.hbs` should be easier to extend #19

Closed tylersticka closed 8 years ago

tylersticka commented 8 years ago

This is a follow-up to my misguided cloudfour/drizzle-builder#81 issue.

The current blank.hbs layout looks like this:

<!DOCTYPE html>
<html class="{{htmlclass}}">
  <head>
    {{#block "head"}}
      {{!-- (etc.) --}}
    {{/block}}
  </head>
  {{#block "wrapper"}}
    <body>
      {{#block "body"}}

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

The trouble is that if you want to extend this template and append things to body like so:

{{#extend "blank"}}
  {{#content "body" mode="append"}}
    <p>Hello world!</p>
  {{/content}}
{{/extend}}

It will not work. This appears to be because the body block is nested within the wrapper block.

My feeling is that we should restructure blank.hbs so there's more consistency between the HTML class, body class, head and what are essentially "foot" assets:

<!DOCTYPE html>
<html class="{{htmlclass}}">
  <head>
    {{#block "head"}}
      {{!-- (this part stays the same) --}}
    {{/block}}
  </head>
  <body class="{{bodyclass}}">
    {{#block "body"}}

    {{/block}}
    {{#block "foot"}}

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

This would allow us to specify bodyclass the same way as htmlclass, and we could pass along <script> elements and other resources with {{#content "foot"}}{{/content}}. But there may be other ways to pull this off!

@mrgerardorodriguez @erikjung

gerardo-rodriguez commented 8 years ago

@tylersticka @erikjung I'm assuming this could be done before with Fabricator?

I'm testing this right now and have this as my setup:

blank.hbs

<!DOCTYPE html>
<html class="{{htmlclass}}">
  <head>
    {{#block "head"}}
       {{!-- (same as before) --}}
    {{/block}}
  </head>
  <body class="{{bodyclass}}">
    {{#block "firstbody"}}

    {{/block}}
    {{#block "foot"}}

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

default.hbs

{{#extend "blank" htmlclass="drizzle-Layout" bodyclass="drizzle-Layout-body"}}
  {{#content "head" mode="append"}}
    <link rel="stylesheet" href="{{baseurl}}/assets/drizzle/styles/drizzle.css">
    <link rel="stylesheet" href="{{baseurl}}/assets/toolkit/styles/toolkit.css">
  {{/content}}
  {{#content "firstbody"}}
      {{> drizzle.nav}}
      <div class="drizzle-Layout-main drizzle-u-cf">
        <div class="drizzle-u-pad drizzle-u-rhythm">
          {{#block "secondbody"}}

          {{/block}}
        </div>
      </div>
      <script src="{{baseurl}}/assets/drizzle/scripts/drizzle.js"></script>
      <script src="{{baseurl}}/assets/toolkit/scripts/toolkit.js"></script>
  {{/content}}
{{/extend}}

collection.hbs

{{#extend "default"}}
  {{#content "secondbody"}}
    <h1>{{name}}</h1>
    {{#each patterns}}
      {{> drizzle.item}}
    {{/each}}
  {{/content}}
{{/extend}}

With this setup, the secondbody content in collection.hbs is not showing up at all.

erikjung commented 8 years ago

@mrgerardorodriguez

I'm assuming this could be done before with Fabricator?

No, we weren't using the layout helpers for page-level templates with that setup (there was no extending of layouts).

Here is an example of the nesting situation working as expected. Not sure why we're running into issues with our stuff.

https://tonicdev.com/erikjung/5719c9333749a112004f164b

I recommend that we rework things to not require any nested blocks.

tylersticka commented 8 years ago

FWIW, if we need to make the layout extensions more shallow and rely more on partials instead, I'm open to that. I just didn't expect to hit a wall with layout extensions the first time I made one!

tylersticka commented 8 years ago

After chatting with @mrgerardorodriguez and getting closer to a PR for the thing I'm working on, I'm actually leaning toward a simple partial being a better way of doing this FWIW. 😆

This issue might still be an issue. Just wanted to mention that in case it takes the pressure off.

gerardo-rodriguez commented 8 years ago

@tylersticka I was trying to replicate your attempt outside of our environment. Does this match what you were expecting?

https://tonicdev.com/gerardorodriguez/nested-handlebars-block

tylersticka commented 8 years ago

@mrgerardorodriguez It appears to, yes! 😮

gerardo-rodriguez commented 8 years ago

Hmm...interesting. It's almost as if there is something special within our setup and the "body" block within blank.hbs.

erikjung commented 8 years ago

@tylersticka @mrgerardorodriguez

I think the flaw may here:

  {{#content "firstbody"}}
      {{> drizzle.nav}}
      <div class="drizzle-Layout-main drizzle-u-cf">
        <div class="drizzle-u-pad drizzle-u-rhythm">
          {{#block "secondbody"}}

Defining the block within the content. We do this, yet I don't see any examples of this in the helper docs: https://github.com/shannonmoeller/handlebars-layouts#helpers

erikjung commented 8 years ago

Also, these test examples are valuable: https://github.com/shannonmoeller/handlebars-layouts/tree/master/test

tylersticka commented 8 years ago

Defining the block within the content. We do this, yet I don't see any examples of this in the helper docs: https://github.com/shannonmoeller/handlebars-layouts#helpers

I do see it referenced in this issue, though via helpers other than {{#content}}: https://github.com/shannonmoeller/handlebars-layouts/issues/6

tylersticka commented 8 years ago

And the deep-extend test example (see @erikjung's link above) seems to be doing all kinds of crazy stuff across multiple extension depths.

gerardo-rodriguez commented 8 years ago

@erikjung @tylersticka Doesn't this example in tonicdev prove, though, that what Tyler wanted to do is possible?

gerardo-rodriguez commented 8 years ago

BTW, looking back at my first example, we should disregard that. It is inaccurate and not what @tylersticka was trying to do.

gerardo-rodriguez commented 8 years ago

And the deep-extend test example (see @erikjung's link above) seems to be doing all kinds of crazy stuff across multiple extension depths.

Yes, indeed.

tylersticka commented 8 years ago

Doesn't this prove, though, that what Tyler wanted to do is possible?

Yes, what you shared in that comment matches what I wanted to do. And it appears to be possible.

At this point, I've already accomplished the thing I initially wanted to use layouts for, so I'm not sure how to prioritize. But it certainly seems as if we've lost some of the magic superpowers of handlebars-layouts along the line in this specific case.

gerardo-rodriguez commented 8 years ago

Removing myself from assignment for now since I'm not actively looking to solve this.

erikjung commented 8 years ago

Changes applied by this PR should also be incorporated here: https://github.com/cloudfour/cloudfour.com-patterns/pull/155

megnotarte commented 8 years ago

Small - Med

gerardo-rodriguez commented 8 years ago

Sharing some findings...

If I modify in drizzle-builder line 28 of render/pages.js from this:

{{#content 'body'}}${page.contents}{{/content}}

to this:

{{#content 'body' mode='append'}}${page.contents}{{/content}}

you can successfully extend and use append or prepend with the body block and the appropriate mode value giving the expected result in @tylersticka's original attempt.

I feel uncomfortable making that the fix because it seems to not matter whether I add mode='append' or mode='prepend' to the render/pages.js file. I do not understand why this fixes it at the moment.

gerardo-rodriguez commented 8 years ago

Keep in mind, the issue we are running into only applies to any blocks that are named body. For example:

blank.hbs

<!DOCTYPE html>
<html class="{{htmlclass}}">
  <head>
    {{#block "head"}}
       ...
    {{/block}}
  </head>
  {{#block "wrapper"}}
    <body>
      {{#block "body"}}

      {{/block}}
      {{#block "bodybody"}}

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

another-layout.hbs

{{#extend "blank"}}
  {{#content "body" mode="append"}}
    <p>Currently, this doesn't work.</p>
  {{/content}}

  {{#content "bodybody" mode="append"}}
    <p>This will work just fine, as expected.</p>
  {{/content}}
{{/extend}}

I wanted to point this out to to clear up an earlier assumption we had where we believed the issue was because the body block was nested within the wrapper block.

This example proves our initial hypothesis was incorrect.

gerardo-rodriguez commented 8 years ago

What I've gathered

After doing some more digging, testing as well as chatting with @erikjung, I now have a much better grasp of what is going on.

To help explain, let's use these:

templates/blank.hbs

<!DOCTYPE html>
<html class="{{htmlclass}}">
  <head>
    {{#block "head"}}
      ...
    {{/block}}
  </head>
  {{#block "wrapper"}}
    <body>
      {{#block "body"}}

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

templates/my-custom-layout.hbs

{{#extend "blank"}}
  {{#content "body" mode="append"}}
    <p>We expect this content to be appended.</p>
  {{/content}}
{{/extend}}

pages/test-page.hbs

---
title: Test Page
layout: my-custom-layout
---
<h1>{{title}}</h1>

<p>This is a test page.</p>

The way things are setup right now, the <p>We expect this content to be appended.</p> will never show up because it will be replaced by the content in the pages/test-page.hbs file. This is happening because when drizzle-builder wraps the pages/test-page.hbs content inside of an {{#extend "my-custom-layout"}} it doesn't set a mode value on the {{#content 'body'}} block, therefore, uses the default handlebars-layouts value of replace. (See drizzle-builder line ~28)

Understanding that, I wonder, what do we want to happen? There is nothing wrong with how it is setup as long as we understand and expect it to work the way it does. Are we okay with this? If so, then there is no changes to be made to drizzle-builder.

The current solution

There is a way to do what was originally attempted by @tylersticka. To get what we intended to do (not replace <p>We expect this content to be appended.</p>), we need to do this:

pages/test-page.hbs

---
title: Test Page
layout: my-custom-layout
---

{{#extend "my-custom-layout"}}
  {{#content "body" mode="append"}}
    <h1>{{title}}</h1>

    <p>This is a test page.</p>
  {{/content}}
{{/extend}}

This works as was originally intended. What are your thoughts @tylersticka @erikjung?

tylersticka commented 8 years ago

So to summarize the core issue, if you use {{#content mode="append"}}, followed by {{#content mode="replace"}} (which is the default), it will replace everything (default, appended, prepended, etc.). I guess that makes sense.

In that case, I kinda think one of my original suggestions might still make sense:

<!DOCTYPE html>
<html class="{{htmlclass}}">
  <head>
    {{#block "head"}}
      {{!-- (this part stays the same) --}}
    {{/block}}
  </head>
  <body class="{{bodyclass}}">
    {{#block "body"}}

    {{/block}}
    {{#block "foot"}}

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

The main reason I think this might be a good idea is that I think it will be really common to want to shove content below the body elements. This is where most <script> elements will have to go, for example.

I think your suggestion is a good one if you want to do anything fancier than that. But I think it would be nice to have a block ready for the most common use-case.

erikjung commented 8 years ago

There is nothing wrong with how it is setup as long as we understand and expect it to work the way it does. Are we okay with this? If so, then there is no changes to be made to drizzle-builder.

@mrgerardorodriguez To clarify...

What is the fix for the original issue? https://github.com/cloudfour/drizzle-builder/issues/81

Specifically, what would need to change in the page itself (not in any template) in order for @tylersticka's hypothetical "Hello world" example to work?

gerardo-rodriguez commented 8 years ago

What is the fix for the original issue? cloudfour/drizzle-builder#81

@erikjung: Regarding the "Hello world" example in https://github.com/cloudfour/drizzle-builder/issues/81, if we assume:

Given a hypothetical test.hbs layout that extends blank.hbs:

{{#extend "blank"}}
  {{#content "body" mode="append"}}
    world
  {{/content}}
{{/extend}}

You would update the page from this:

---
title: Test
layout: test
---

Hello

To this:

---
title: Test
layout: test
---

{{#extend "test"}}
  {{#content "body" mode="prepend"}}
    Hello
  {{/content}}
{{/extend}}
gerardo-rodriguez commented 8 years ago

So to summarize the core issue, if you use {{#content mode="append"}}, followed by {{#content mode="replace"}} (which is the default), it will replace everything (default, appended, prepended, etc.).

@tylersticka Yes and no? So, in the example I posted today, here's what happens.

templates/my-custom-layout.hbs sets its mode to append and also has content.

pages/test-page.hbs (with only front matter and no {{#extend}}) has content and will have a mode of replace because of how drizzle-builder is setup right now.

So this means the contents of pages/test-page.hbs will replace the contents of templates/my-custom-layout.hbs, which will then be appended to the templates/blank.hbs body block. Did that make sense? I may not be articulating very well.

Keep in mind, all of this we are discussing is only for the body block since that is what drizzle-builder is looking for.

erikjung commented 8 years ago

You would update the page from this

@mrgerardorodriguez IMO, this seems reasonable enough. Maybe I'm just struggling to come up with a more solid way to improve the template extension in general, but if this issue can be bypassed altogether with this small adjustment, maybe that's sufficient for now?

tylersticka commented 8 years ago

I think that's sufficient for now, though as I said before, I do think adding a blank foot block to blank.hbs would be a nice usability default.

gerardo-rodriguez commented 8 years ago

I think that's sufficient for now

I'm fine with this as well for now.

though as I said before, I do think adding a blank foot block to blank.hbs would be a nice usability default

I'm fine by this, I can make these changes. Any objections @erikjung?

erikjung commented 8 years ago

@mrgerardorodriguez No objections from me.

gerardo-rodriguez commented 8 years ago

@tylersticka Should this be considered "done"?

tylersticka commented 8 years ago

@mrgerardorodriguez I think so, especially since #53 exists now (which will carry the torch of this issue).