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

Properly format boolean values in attribute tables #4602

Closed
AlexVPopov opened this issue Aug 25, 2016 · 5 comments · Fixed by #4938
Closed

Properly format boolean values in attribute tables #4602

AlexVPopov opened this issue Aug 25, 2016 · 5 comments · Fixed by #4938

Comments

@AlexVPopov
Copy link
Contributor

I propose the following change:

def pretty_format(object)
  case object
  when String, Numeric, Symbol, Arbre::Element
    object.to_s
  when TrueClass, FalseClass
    status_tag value
  when Date, Time
    localize object, format: active_admin_application.localize_format
  else
    if defined?(::ActiveRecord) && object.is_a?(ActiveRecord::Base) ||
       defined?(::Mongoid)      && object.class.include?(Mongoid::Document)
      auto_link object
    else
      display_name object
    end
  end
end

in ActiveAdmin::ViewHelpers::DisplayHelper

The idea is that currently the value of boolean columns is displayed via status_tag and the result of methods, that return true/false, is displayed via a ✔/✗. In an attributes table, where some of the rows are for boolean columns and some of the rows are for methods returning booleans, the table looks inconsistent.

Would such a PR be accepted?

@Fivell
Copy link
Member

Fivell commented Feb 12, 2017

@AlexVPopov , what about null/nil values?

@AlexVPopov
Copy link
Contributor Author

Maybe we can add NilClass to the first case?

@Fivell
Copy link
Member

Fivell commented Feb 13, 2017

@AlexVPopov, I didn't test but according to current implementations nulls are displayed as status_tag 'false' if database column is boolean and just nil in all other cases ..

@AlexVPopov
Copy link
Contributor Author

@Fivell I need to take and do a research on this, as it's been a long time since I proposed this.

@AlexVPopov
Copy link
Contributor Author

@Fivell nice, thank you.

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 a pull request may close this issue.

3 participants