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

Feature: Restructure blank.hbs #52

Merged
merged 4 commits into from May 13, 2016

Conversation

gerardo-rodriguez
Copy link
Member

Per the comment here, this PR restructures the blank.hbs template and adds a blank foot block.

cc: @tylersticka @erikjung

@erikjung
Copy link
Contributor

Should the foot block be inside of wrapper?

@gerardo-rodriguez
Copy link
Member Author

gerardo-rodriguez commented May 13, 2016

Should the foot block be inside of wrapper?

Great question @erikjung!

I initially put the foot block inside the wrapper. In doing so, though, I realized that as soon as you extend the blank.hbs template and pass along content via {{#content "wrapper"}}, this content replaces everything within the blank.hbs wrapper block, including the foot block. This means I can no longer use {{#content "foot"}} in my extended template which I assumed was the goal.

If this is an incorrect assumption, we can treat the foot block the same way we treat the body block. As we currently have it setup, when we extend the blank.hbs template and pass content using the {{#content "wrapper"}} we then create a new body block within this wrapper. We could also create a new foot block a second time in the extended template as well. I guess, to me, it feels a bit odd to ditch the foot block from the blank.hbs template only to recreate a foot block in the extended template a second time.

What are your thoughts? If this isn't making sense, let me know and I can paste in some code examples. Thanks! :)

@erikjung
Copy link
Contributor

@mrgerardorodriguez I'm not sure if it's correct or not anymore 😄

I'll defer to @tylersticka for an opinion on that placement of that foot block.

@tylersticka
Copy link
Member

I think foot should go after wrapper, for the reasons @mrgerardorodriguez mentioned. If we put it inside of wrapper, we may as well just remove it altogether.

(I actually wish wrapper were named body and body were named main. But I'm pretty sure that change is too low-level to prioritize now.)

@gerardo-rodriguez
Copy link
Member Author

(I actually wish wrapper were named body and body were named main. But I'm pretty sure that change is too low-level to prioritize now.)

Good point @tylersticka. I considered a similar change like this as well. It may actually not be too bad to make that change. That being said, perhaps we should make that a separate issue/task?

@tylersticka
Copy link
Member

It may actually not be too bad to make that change.

I assumed it would be larger because it would require a change to the builder, but I'd love to be wrong about that.

That being said, perhaps we should make that a separate issue/task?

Couldn't agree more! 👍

@gerardo-rodriguez
Copy link
Member Author

I assumed it would be larger because it would require a change to the builder, but I'd love to be wrong about that.

It does require a change in to the builder, but the way it was written, it doesn't seem like a huge task. :)

Couldn't agree more! 👍

Was this a "LGTM" or a "I agree enthusiastically? I couldn't tell. 😁

@tylersticka
Copy link
Member

Both, I guess? ¯_(ツ)_/¯

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

Successfully merging this pull request may close these issues.

None yet

3 participants