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

Tag option for sets the attribute when value is a string or symbol #49

Merged
merged 1 commit into from
Feb 14, 2016

Conversation

ramontayag
Copy link

We didn't change the option to avoid a breaking change. If changing the usage of for should change completely, a deprecation warning can easily be shown. This gets labels working fine at least.

fixes #18

@@ -20,7 +20,10 @@ def build(*args)
attributes = extract_arguments(args)
self.content = args.first if args.first

set_for_attribute(attributes.delete(:for))
for_value = attributes[:for]
unless for_value.kind_of?(String) || for_value.kind_of?(Symbol)

Choose a reason for hiding this comment

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

Prefer Object#is_a? over Object#kind_of?.

…ctiveadmin#18]

We didn't change the option to avoid a breaking change.

fixes activeadmin#18
@zorab47
Copy link

zorab47 commented Jan 8, 2016

It kinda makes the for attribute a little ✨ magical ✨, but it is a nice solution. I'm in favor of Arbre building HTML nodes over supporting ActiveModel anyway.

@seanlinsley
Copy link
Contributor

I would merge this, but I'm not really sure I understand the existing behavior. Also the test suite appears to be failing (but it doesn't seem to be caused by this PR).

@ramontayag
Copy link
Author

IIRC, when you add for, it assumes it's an ActiveModel/ActiveRecord object and adds a dom_id to the class (i.e. post_32)

@timoschilling
Copy link
Member

I know about this problem, this is not a nice way but if we support the Rails for functionality, it is the only way. This can be merged.

timoschilling added a commit that referenced this pull request Feb 14, 2016
Tag option `for` sets the attribute when value is a string or symbol
@timoschilling timoschilling merged commit 6d902ae into activeadmin:master Feb 14, 2016
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.

Unable to set the "for" attribute on tags
5 participants