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

Add option to "undecorate" resource for forms #2085

Merged
merged 1 commit into from
Apr 10, 2013
Merged

Add option to "undecorate" resource for forms #2085

merged 1 commit into from
Apr 10, 2013

Conversation

amiel
Copy link
Contributor

@amiel amiel commented Apr 10, 2013

This pull-request adds the following option:

ActiveAdmin.register MyModel do
  decorate_with MyModelDecorator

  # Calls `model` on resource before calling semantic_form_for
  form decorate: false do
    # ...
  end

end

This also adds more coverage for ActiveAdmin::ViewHelpers::FormHelper.active_admin_form_for (specs not specific to this pull-request).

See discussion at #2057.

options[:undecorate_with] = :model if options.delete(:undecorate)
if (undecorate_method = options.delete(:undecorate_with))
resource = resource.send(undecorate_method)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

These four lines of code are really hard to follow. It seems like all you want is this:

if should = options.delete :undecorate
  resource = resource.send should==true ? :model : should
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daxter I agree, but does the api concept even make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

So the changes I made above removed the (unnecessary) undecorate_with option, providing the same functionality through the undecorate option.

Whether or not you adopt those changes, could you please explain what happens when you call .model, and what sorts of other options someone might want to pass in?

@macfanatic
Copy link
Contributor

Personally, I would rather see the simplest DSL:

ActiveAdmin.register Post do
  decorate_with PostsDecorator
  form decorated: false, partial: 'form'
end

Accept true/false & move on. Why would we need to accept a symbol as a method name to call on the decorator to get the original resource, if you can just get the resource from the decorator when you pass false in? Technically we are not tying you to Draper, but I think using the :model method to get the resource instance from the decorator is a safe assumption.

@seanlinsley
Copy link
Contributor

+1 on the decorated: false syntax. @amiel, is there any other value you might want to pass in?

@amiel
Copy link
Contributor Author

amiel commented Apr 10, 2013

@daxter @macfanatic that works for me, I like it. I just wasn't sure if the customized method call might be useful for something else...

@seanlinsley
Copy link
Contributor

If such a time comes, we can always change it later :)

On Apr 10, 2013, at 11:24 AM, Amiel Martin notifications@github.com wrote:

@daxter @macfanatic that works for me, I like it. I just wasn't sure if the customized method call might be useful for something else...


Reply to this email directly or view it on GitHub.

@amiel
Copy link
Contributor Author

amiel commented Apr 10, 2013

@daxter +1 for YAGNI. Ok, I'll change the documentation to include this, and I think it should be ready to go.

macfanatic added a commit that referenced this pull request Apr 10, 2013
Add option to "undecorate" resource for forms
@macfanatic macfanatic merged commit 01af081 into activeadmin:master Apr 10, 2013
@amiel amiel deleted the forms-without-decorators branch April 10, 2013 18:46
@amiel
Copy link
Contributor Author

amiel commented Apr 10, 2013

Thanks guys! :)

@seanlinsley
Copy link
Contributor

Thanks to you for putting in the effort

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.

3 participants