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

Allow partials accept blocks #1504

Closed
lolmaus opened this Issue Nov 27, 2013 · 13 comments

Comments

Projects
None yet
2 participants
@lolmaus
Contributor

lolmaus commented Nov 27, 2013

It is awesome how Jade allows you to call mixins with different options and content blocks.

Jade:

mixin article(title)
  .article
    .article-wrapper
      h1= title
      if block
        block
      else
        p No content provided

+article('Hello world')

+article('Good news, everyone!')
  p This is my
  p Amazing article

The same functionality is found in Sass, but surprisingly is missing in Padrino's template engines. Padrino has partials functionality abstracted out of template engines into a Padrino helper, and Padrino partial helper does not accept content blocks! :(

Middleman introduces the wrap_layout helper that lets you pass a block to a layout (which in this case is almost the same as partial). Unfortunately, it does not accept options, so you can't, for example, pass local variables. :( Also, migrating partials into templates would be an ugly workaround.

I've managed to mimic the desired funcitonality by leveraging Padrino's capture_html. Here's an already working example (in Middleman):

Partial _test.haml:

- if defined? block
  = block
- else
  Welcome, friend

Page index.html.haml:

// Calling the partial without a block
= partial 'test'

// Preparing a block
- captured_html = capture_html do
  %h2 Wassup!

// Calling the partial with a block
= partial 'test', locals: { block: captured_html }

But this approach is also kinda ugly because it requires to write a block before partial call and use an ad-hoc variable to store it.

Note: you can't use content_for for this purpose because contents of content_for is accumulated rather than redefined when you use it multiple times.

I also tried creating a helper instead of a partial-that-accepts-a-block. It made using the helper-partial awesomely clean, just as i request. But the HTML of the helper-partial had to be written in Ruby strings and assembled via content_tag in reverse order, which is very ugly. :(

So i suggest the following syntax for Padrino partials. It is just a concept and open for discussion.

Haml:

// Calling the partial without a block
= partial 'title', class: 'first-title'

// Calling the same partial with a block
= partial 'test', class: 'second-title' do
  %h2 Wassup!

Also, please have a look at how DerbyJS allows passing multiple blocks to a partial.

DerbyJS + Jade:

  app:shared:header // Calling the partial
    @left
      label
        input(type='checkbox', checked='{_page.user.data.prof}')/
        | Professor (Create new game)
    @right
      a.icon-admin(href='/admin') Admin Panel

I wonder whether something similar can be done in Padrino without falling back to the capture_html workaround.

@ujifgc ujifgc closed this in 67f03e3 Nov 27, 2013

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 27, 2013

Member

@lolmaus thank you for your suggestion. Does the commit do what you wanted?

Member

ujifgc commented Nov 27, 2013

@lolmaus thank you for your suggestion. Does the commit do what you wanted?

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Nov 27, 2013

Contributor

This is awesome, @ujifgc! I can't believe how quick and kind you've been to implement it.

Unfortunately, this bug in Middleman prevents me from trying it out. But it seems to be exactly what i was hoping for. Thank you Igor!

When is this going to hit RubyGems?

Contributor

lolmaus commented Nov 27, 2013

This is awesome, @ujifgc! I can't believe how quick and kind you've been to implement it.

Unfortunately, this bug in Middleman prevents me from trying it out. But it seems to be exactly what i was hoping for. Thank you Igor!

When is this going to hit RubyGems?

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Nov 27, 2013

Contributor

I tried manually creating a helper with your code in a sandbox Middleman project and it didn't work out. :(

yield within a partial produces the following error:

NameError at /
undefined local variable or method `layout_dir' for #Middleman::Application:0x32188944
raise "Tried to render a layout (calls yield) at #{path} like it was a template. Non-default layouts need to be in #{source}/#{layout_dir}."

Contributor

lolmaus commented Nov 27, 2013

I tried manually creating a helper with your code in a sandbox Middleman project and it didn't work out. :(

yield within a partial produces the following error:

NameError at /
undefined local variable or method `layout_dir' for #Middleman::Application:0x32188944
raise "Tried to render a layout (calls yield) at #{path} like it was a template. Non-default layouts need to be in #{source}/#{layout_dir}."

@ujifgc ujifgc reopened this Nov 27, 2013

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Nov 27, 2013

Contributor

@ujifgc Igor, are you going to work on this further?

I'm not good in Ruby enough to try finishing this on my own. I even failed to find where yield is defined.

Contributor

lolmaus commented Nov 27, 2013

@ujifgc Igor, are you going to work on this further?

I'm not good in Ruby enough to try finishing this on my own. I even failed to find where yield is defined.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 28, 2013

Member

Yes, later. Not right away.

Member

ujifgc commented Nov 28, 2013

Yes, later. Not right away.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 28, 2013

Member

@lolmaus could you sketch up a broken test app and publish it at github? I never used middleman before.

Member

ujifgc commented Nov 28, 2013

@lolmaus could you sketch up a broken test app and publish it at github? I never used middleman before.

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Nov 29, 2013

Contributor

Here you go: https://github.com/lolmaus/middleman-padrino-test-partial-block

It's not producing an error as i had said (have you added commits?), but it doesn't work as expected.

I've got a partial:

%h1 prefix
.haml-block
  = yield
%h3 postfix

I call it like this:

= partial 'foo' do
  Am i the only one around here who has this failing?

Resulting HTML:

    Am i the only one around here who has this failing?
<h1>prefix</h1>
<div class="haml-block">
  0
</div>
<h3>postfix</h3>
Contributor

lolmaus commented Nov 29, 2013

Here you go: https://github.com/lolmaus/middleman-padrino-test-partial-block

It's not producing an error as i had said (have you added commits?), but it doesn't work as expected.

I've got a partial:

%h1 prefix
.haml-block
  = yield
%h3 postfix

I call it like this:

= partial 'foo' do
  Am i the only one around here who has this failing?

Resulting HTML:

    Am i the only one around here who has this failing?
<h1>prefix</h1>
<div class="haml-block">
  0
</div>
<h3>postfix</h3>

@ujifgc ujifgc closed this in 9121a5a Nov 29, 2013

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 29, 2013

Member

I fixed the issue and added a test.

I'm aware that middleman stays broken but it's because recent improvements to padrino rendering extensions. The problem is, as I understand, the monkey patch https://github.com/middleman/middleman/blob/master/middleman-core/lib/middleman-more/core_extensions/default_helpers.rb#L74 that expects old code.

Member

ujifgc commented Nov 29, 2013

I fixed the issue and added a test.

I'm aware that middleman stays broken but it's because recent improvements to padrino rendering extensions. The problem is, as I understand, the monkey patch https://github.com/middleman/middleman/blob/master/middleman-core/lib/middleman-more/core_extensions/default_helpers.rb#L74 that expects old code.

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Nov 29, 2013

Contributor

Do you see how Middleman can be fixed?

Contributor

lolmaus commented Nov 29, 2013

Do you see how Middleman can be fixed?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 29, 2013

Member

Investigate the reasons of monkey patching, remove them. Or patch to use
new storage of rendering extensions.
On Nov 29, 2013 11:26 AM, "lolmaus" notifications@github.com wrote:

Do you see how Middleman can be fixed?


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/1504#issuecomment-29500076
.

Member

ujifgc commented Nov 29, 2013

Investigate the reasons of monkey patching, remove them. Or patch to use
new storage of rendering extensions.
On Nov 29, 2013 11:26 AM, "lolmaus" notifications@github.com wrote:

Do you see how Middleman can be fixed?


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/1504#issuecomment-29500076
.

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Nov 29, 2013

Contributor

@ujifgc, Igor, can you please open an issue ticket for Middleman, explain the problem in detail and state your suggestions? I don't want to play Chinese whispers here.

Contributor

lolmaus commented Nov 29, 2013

@ujifgc, Igor, can you please open an issue ticket for Middleman, explain the problem in detail and state your suggestions? I don't want to play Chinese whispers here.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Nov 29, 2013

Member

No, I'm sorry, I don't use Middleman. I looked at their code and can't figure out what are they trying to do with our helpers and why. I can guess that's some patching needed on version 0.11.4.

Member

ujifgc commented Nov 29, 2013

No, I'm sorry, I don't use Middleman. I looked at their code and can't figure out what are they trying to do with our helpers and why. I can guess that's some patching needed on version 0.11.4.

@lolmaus

This comment has been minimized.

Show comment
Hide comment
@lolmaus

lolmaus Nov 29, 2013

Contributor

Thank you @ujifgc. ^__^

Contributor

lolmaus commented Nov 29, 2013

Thank you @ujifgc. ^__^

mitchellhenke pushed a commit to mitchellhenke/padrino-framework that referenced this issue Nov 30, 2013

Ortuna added a commit to Ortuna/padrino-framework that referenced this issue Jan 17, 2014

Ortuna added a commit to Ortuna/padrino-framework that referenced this issue Jan 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment