-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Render helper with block containing Arbre element. #64
Render helper with block containing Arbre element. #64
Conversation
lib/arbre/rails/template_handler.rb
Outdated
# and https://github.com/rails/rails/pull/18024#commitcomment-8975180 | ||
value = value.to_s if value.is_a?(Arbre::Element) | ||
|
||
if string = buffer.presence || value and string.is_a?(String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use && instead of and.
lib/arbre/rails/template_handler.rb
Outdated
@@ -1,3 +1,19 @@ | |||
class ActionView::Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing frozen string literal comment.
Use nested module/class definitions instead of compact style.
@varyonic , is it still WIP ? |
b79aec4
to
6fc0d29
Compare
lib/arbre/element.rb
Outdated
within(Element.new) { s = helpers.send(name, *args, &block) } | ||
s.is_a?(Element) ? s.to_s : s | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra empty line detected at class body end.
lib/arbre/element.rb
Outdated
# We do not want such elements added to self, so we push a dummy | ||
# current_arbre_element. | ||
def helper_capture(name, *args, &block) | ||
s = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
lib/arbre/element.rb
Outdated
@@ -177,11 +177,21 @@ def method_missing(name, *args, &block) | |||
elsif assigns && assigns.has_key?(name) | |||
assigns[name] | |||
elsif helpers.respond_to?(name) | |||
helpers.send(name, *args, &block) | |||
value = helper_capture(name, *args, &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless assignment to variable - value.
get "/test/render_page_with_helpers" | ||
expect(response).to be_success | ||
expect(body).to eq <<-EOS | ||
<span>before h1 link</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.
6fc0d29
to
3f467ab
Compare
get "/test/render_page_with_helpers" | ||
expect(response).to be_success | ||
expect(body).to eq <<~EOS | ||
<span>before h1 link</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 2 spaces for indentation in a heredoc by using <<~ instead of <<~.
lib/arbre/rails/template_handler.rb
Outdated
@@ -1,3 +1,20 @@ | |||
# frozen_string_literal: true | |||
ActionView::Base.class_eval do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an empty line after magic comments.
6fc0d29
to
c5840c0
Compare
# Override to handle Arbre elements inside helper blocks. | ||
# See https://github.com/rails/rails/issues/17661 | ||
# and https://github.com/rails/rails/pull/18024#commitcomment-8975180 | ||
value = value.to_s if value.is_a?(Arbre::Element) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied ActionView::Helpers::CaptureHelper.capture and added this line to convert Arbre::Element#to_s so the output is not lost.
I was overcomplicating things by trying to eliminate text_node. link_to works with text_node and now the arbre inside the link_to block is captured and displayed in the order expected. |
c5840c0
to
82fb896
Compare
get "/test/render_page_with_helpers" | ||
expect(response).to be_success | ||
expect(body).to eq <<EOS | ||
<span>before h1 link</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 2 spaces for indentation in a heredoc by using <<~ instead of <<.
82fb896
to
ed8db93
Compare
ed8db93
to
fbb048e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! 😄 Could you write a changelog entry? It might also be valuable to document how to use Rails helpers with Arbre as a new section in the README.
@@ -1,3 +1,21 @@ | |||
# frozen_string_literal: true | |||
|
|||
ActionView::Base.class_eval do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be moved to its own file, named something like lib/arbre/rails/capture.rb
One concern with the monkeypatch is that it doesn't call the original implementation, so another gem that overrode This might work as an alternative: ActionView::Base.prepend Module.new {
def capture(*args)
super *args do
value = yield *args
value = value.to_s if value.is_a? Arbre::Element
value
end
end
} I'm not sure if |
I'm going to take this as approved and consider the suggestions for later. |
This change implements activeadmin/arbre#64 https://github.com/activeadmin/arbre/pull/64/files which was reverted. The initial PR was reverted because you could potentially get duplicate renders in certain situations. The workaround I can see is to return nil from the block if this occurs. I think this is an acceptable workaround for this library.
This PR fixes #46 following the first option suggested by Sean. It monkey-patches ActionView::Base#capture so that the output of Arbre elements is not lost inside a Rails helper block, and it modifies Arbre::Element#method_missing to capture the output of Rails helper calls into text_nodes.