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

Update yield_content to append default to ERB template buffer #1500

Merged
merged 7 commits into from Dec 9, 2018

Conversation

jkowens
Copy link
Member

@jkowens jkowens commented Nov 30, 2018

Resolves #1480. For ERB, using a block with an expression printing tag is invalid syntax. This PR allows yield_content to append directly to the template buffer using a non-printing tag.

@jkowens
Copy link
Member Author

jkowens commented Nov 30, 2018

@programmarchy, @krelly any chance you could vet this PR and make sure it fixes the issues you found?

Copy link

@krelly krelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm this fixes the issue.

@jkowens
Copy link
Member Author

jkowens commented Dec 2, 2018

Thanks @krelly 👍🏻

@namusyaka namusyaka self-requested a review December 8, 2018 14:48
@namusyaka
Copy link
Member

Will review soon

Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work @jkowens !
I've put a few comments, please take a look.

@@ -96,8 +96,7 @@ def capture(*args, &block)
result = block[*args]
elsif current_engine == :erb || current_engine == :slim
@_out_buf, _buf_was = '', @_out_buf
block[*args]
result = eval('@_out_buf', block.binding)
result = block.call(*args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should try to get result from @_out_buf unless returned value is empty.

raw = block.call(*args)
captured = block.binding.eval('@_out_buf')
result = captured.empty? ? raw : captured

See: https://github.com/padrino/padrino-framework/blob/02feacb6afa9bce20c1fb360df4dfd4057899cfc/padrino-helpers/lib/padrino-helpers/output_helpers/abstract_handler.rb#L36

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call 👍

sinatra-contrib/lib/sinatra/content_for.rb Show resolved Hide resolved
content_blocks[key.to_sym].map { |b| capture(*args, &b) }.join
def yield_content(key, *args, &block)
if block_given? && !content_for?(key)
capture(*args, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use capture even if unnecessary? This makes yield_content performance be slowed a bit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I was trying to eliminate the need to check for haml here, but will revert back to previous version.

Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good work!

@namusyaka namusyaka merged commit 8a7da30 into sinatra:master Dec 9, 2018
@jkowens
Copy link
Member Author

jkowens commented Dec 10, 2018

@namusyaka thanks for helping me work through this one!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 6, 2019
### sinatra-contrib

* Add `flush` option to `content_for` [#1225](sinatra/sinatra#1225) by Shota Iguchi

* Drop activesupport dependency from sinatra-contrib [#1448](sinatra/sinatra#1448)

* Update `yield_content` to append default to ERB template buffer [#1500](sinatra/sinatra#1500) by Jordan Owens
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