Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Only show empty_value for attributes_table row if the row is actually empty #1479

Closed
wants to merge 2 commits into from

3 participants

James McKinney Don't Add Me To Your Organization a.k.a The Travis Bot Sean Linsley
James McKinney

If you do:

ActiveAdmin.register ClassName do
  show do
    attributes_table do
      row :attribute_name do |x|
        div "some content"
        if some_condition
          div "other content"
        end
      end
    end
  end
end

and if some_condition evaluates to false, then empty_value will be rendered after "some_content", even though the row is not empty! You can get around this by doing:

row :attribute_name do |x|
  div "some content"
  if some_condition
    div "other content"
  end
  "foobar"
end

because "foobar" won't be rendered, but that seems like a cludgy workaround. This pull request fixes this, so that an attributes_table row can return nil. The new code checks whether any content was created by the block passed to row, instead of checking the block's return value.

Specs included.

James McKinney

This is a re-opening of #1460. Copying comment from that issue:

There is an error in the attributes_table_spec tests. The post ID is set to 1, the test compares it to "2" and yet the test passes. This is because the let statements need to be in the most deeply nested describe block. This sort of error can be avoided by not trying to DRY the tests too much. Removing the let statements and just writing set[0] instead of title, etc. is an alternative solution to this error in the tests.

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged 89c27ff into ffef502).

Sean Linsley seanlinsley commented on the diff
lib/active_admin/views/components/attributes_table.rb
@@ -50,7 +51,7 @@ def content_for(attr_or_proc)
content_for_attribute(attr_or_proc)
end
value = pretty_format(value)
- value == "" || value == nil ? empty_value : value
+ (value == "" || value == nil) && previous == current_arbre_element.to_s ? empty_value : value
Sean Linsley Owner

Can't we rely on Active Support here?

value.blank? && previous == current_arbre_element.to_s ? empty_value : value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sean Linsley seanlinsley referenced this pull request from a commit
Sean Linsley seanlinsley only show empty_value for attr table if actually empty
Applying @jpmckinney's PR #1479 which never made it in.
600b539
Sean Linsley
Owner

Merged in as of 600b539

Sean Linsley seanlinsley closed this
James McKinney jpmckinney deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
3  lib/active_admin/views/components/attributes_table.rb
View
@@ -43,6 +43,7 @@ def empty_value
end
def content_for(attr_or_proc)
+ previous = current_arbre_element.to_s
value = case attr_or_proc
when Proc
attr_or_proc.call(@record)
@@ -50,7 +51,7 @@ def content_for(attr_or_proc)
content_for_attribute(attr_or_proc)
end
value = pretty_format(value)
- value == "" || value == nil ? empty_value : value
+ (value == "" || value == nil) && previous == current_arbre_element.to_s ? empty_value : value
Sean Linsley Owner

Can't we rely on Active Support here?

value.blank? && previous == current_arbre_element.to_s ? empty_value : value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
def content_for_attribute(attr)
22 spec/unit/views/components/attributes_table_spec.rb
View
@@ -46,8 +46,16 @@
row("Body") { post.body }
end
}
- }
-
+ },
+ "when you create each row with a custom block that returns nil" => proc {
+ render_arbre_component(assigns) {
+ attributes_table_for post do
+ row("Id") { text_node post.id; nil }
+ row("Title"){ text_node post.title; nil }
+ row("Body") { text_node post.body; nil }
+ end
+ }
+ },
}.each do |context_title, table_decleration|
context context_title do
let(:table) { instance_eval &table_decleration }
@@ -68,15 +76,15 @@
describe "rendering the rows" do
[
- ["Id" , "2"],
+ ["Id" , "1"],
["Title" , "Hello World"],
["Body" , "<span class=\"empty\">Empty</span>"]
].each_with_index do |set, i|
- let(:title){ set[0] }
- let(:content){ set[1] }
- let(:current_row){ table.find_by_tag("tr")[i] }
-
describe "for #{set[0]}" do
+ let(:title){ set[0] }
+ let(:content){ set[1] }
+ let(:current_row){ table.find_by_tag("tr")[i] }
+
it "should have the title '#{set[0]}'" do
current_row.find_by_tag("th").first.content.should == title
end
Something went wrong with that request. Please try again.