Name action items; allow removing them by name #3091

Merged
merged 1 commit into from Oct 24, 2014

Conversation

Projects
None yet
6 participants
Contributor

amiel commented Apr 19, 2014

As discussed in #760, this allows for customizing what of the default action items are shown. It could also be used to allow for customizing action items per request, but that is beyond the scope of this pull-request.

Owner

seanlinsley commented Apr 19, 2014

I'm trying to decide whether it's okay to keep this as an array with optional names... It's a breaking change, but making the name required seems to fit the overall style of the rest of Active Admin.

Contributor

dmitry commented Apr 19, 2014

Hope that #3074 will be also merged before this pull-request. Afterwards it's anyway better to abstract actions should be shown in the index to an array, hash or similar structure.

Contributor

amiel commented Apr 21, 2014

@seanlinsley If you are looking for input on that, my suggestion would be to just add this with a deprecation warning when calling action_item without a name option. Although, it seems to me as though it's only important for the default action items to have a name, but I guess there is something to be said for the consistency of everything having a name...

@dmitry That pull-request looks interesting, but nothing to do with this one. This is about custom action_items, not the actions in the index table.

Contributor

amiel commented Apr 21, 2014

@dmitry no worries; they both have similar names

Contributor

amiel commented May 22, 2014

@seanlinsley have you given this any more thought? Do you want me to rebase?

xhero commented Jul 7, 2014

Any updates on this? For a project I am working on it would be quite handy to remove the new action and substitute it with a custom one

Contributor

amiel commented Jul 14, 2014

@seanlinsley Do you need anything else from me on this one?

Hi @seanlinsley any eta on this?

Cheers, Joel

Owner

timoschilling commented Oct 21, 2014

we can do it this way, so we have a backward compatibility.

def action_item(name, options = {}, &block)
  Deprecation.warn "using `action_item` without a name is deprecated! Use `action_item(:edit)`." unless name
  config.add_action_item(name, options, &block)
end

def add_action_item(name = nil, options = {}, &block)
  self.action_items << ActiveAdmin::ActionItem.new(name, options, &block)
end

def initialize(name, options = {}, &block)
  @options, @block = options, block
  self.name = name
  normalize_display_options!
end
Contributor

amiel commented Oct 21, 2014

I'd be happy to add the deprecation warning. Is that being requested? or is this waiting for a decision on that matter?

Owner

timoschilling commented Oct 21, 2014

The deprecation waring is a way that works with old and new code so we should use it!

Owner

seanlinsley commented Oct 21, 2014

using add_action_item without a name is deprecated! Use `add_action_item(:edit)

That message won't make sense to the end user since the method they called was action_item, not add_action_item

Owner

timoschilling commented Oct 21, 2014

Sorry, I'm not gone up far enough in the call stack. Updated my code example

Contributor

amiel commented Oct 21, 2014

Ok, I've rebased on to master and added the name as the first parameter (optional with warning in the DSL). I can't run the tests right now, so I imagine this will fail. I can deal with that tomorrow, but figured I would get the ball rolling here for review.

Owner

timoschilling commented Oct 21, 2014

Resolve the failing test and I will merge it.

Contributor

amiel commented Oct 21, 2014

@timhabermaas Thanks. I don't have any more time today, but I will try to get to it tomorrow :)

Contributor

amiel commented Oct 23, 2014

Ok, I think this is ready.

Owner

seanlinsley commented Oct 23, 2014

screen shot 2014-10-22 at 9 09 27 pm

...

/me unchecks

Owner

timoschilling commented Oct 23, 2014

@seanlinsley you have turned of @coveralls? 👍

Contributor

amiel commented Oct 23, 2014

Sweet! Let me know if there's anything else you need for this pull-request.

Owner

timoschilling commented Oct 24, 2014

@amiel please smash your first three commits into one, they have to many changes of the signature of action_item, they make the tracking of the real change to hard.

Name action items; allow removing them by name
Ref #760

Name first parameter to action items.
It is still optional in the DSL, but triggers a deprecation warning when not provided.
Contributor

amiel commented Oct 24, 2014

Ok, I squashed all of the commits. I also saw your comment about the action_name signature. I'm pretty sure that change would break backwards compatibility. Could you please confirm if you still want this change?

Owner

timoschilling commented Oct 24, 2014

Sorry you are right.

timoschilling added a commit that referenced this pull request Oct 24, 2014

Merge pull request #3091 from carnesmedia/name-action-items
Name action items; allow removing them by name

@timoschilling timoschilling merged commit 3395dd8 into activeadmin:master Oct 24, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Contributor

amiel commented Oct 24, 2014

Thanks @timoschilling! I'm very happy to see this merged :)

@amiel amiel deleted the carnesmedia:name-action-items branch Oct 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment