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

Batch Action forms #1815

Merged
merged 3 commits into from Nov 23, 2013
Merged

Batch Action forms #1815

merged 3 commits into from Nov 23, 2013

Conversation

seanlinsley
Copy link
Contributor

FYI: this is an Issue that was converted into a Pull Request

I want to add optional forms to Batch Actions; an example use-case being that you want the user to add in a text box the reason why they're taking this action. Since we're already including jQuery UI and it has a similar UI for pop-over forms, a bit of the work has already been done.

What I'm curious about is what you think a good DSL syntax would be for this--should I include a form do |f| block inside a batch_action declaration, or should they be defined much like the new/edit pages are, with something like the below syntax?

batch_form :for => :an_action do |f|
  #...
end

UPDATES

  • The syntax is now:
    batch_action :foo, confirm: 'You sure?', form: {bar: :text} do |ids, inputs|
      inputs[:bar] # access user input from this hash
      # do your stuff!
    end
  • I changed the behaviour of the :confirm option. Passing a string still works, but now if you pass true, it will use a default confirm message.
  • The UI now looks like this:
    Modal styling

@macfanatic
Copy link
Contributor

When I worked with @pcreux on designing the DSL earlier this year, we wanted to leave it open enough to do anything you want with it. So, if you really want to add some DSL as you propose above, I would definitely write it as a separate gem that a user can install as needed (there are already several on rubygems doing this if you haven't looked).

I think this will be a complicated addition to take on top of batch actions, but I'm available if you have any questions.

@seanlinsley
Copy link
Contributor Author

There are add-on gems for Active Admin? 1) where are they and 2) why isn't this mentioned in the docs?
Implementing this doesn't look to be all that complicated--the batch action itself is already a form that's being submitted, and jQuery UI gives us the basic UI functionality--but thanks for the offer @macfanatic :)

@seanlinsley
Copy link
Contributor Author

I'm thinking a better syntax would be:

batch_action :whitelist, :form => {:reason => :text} do |ids, reason|
  # do your stuff!
end

That way the do block is still only used for the "action" part of a batch action.

@macfanatic
Copy link
Contributor

I like this recommendation better.

On Dec 14, 2012, at 10:51 AM, Sean Ian Linsley notifications@github.com wrote:

I'm thinking a better syntax would be:

batch_action :whitelist, :form => {:reason => :text} do |ids, reason|

do your stuff!

end
That way the do block is only used for the "action" in a batch action.


Reply to this email directly or view it on GitHub.

@macfanatic
Copy link
Contributor

Not in the docs because they are from 3rd parties. I have a few up there, just search for "active_admin" and "active admin" on ruby gems.

On Dec 14, 2012, at 10:40 AM, Sean Ian Linsley notifications@github.com wrote:

There are add-on gems for Active Admin? 1) where are they and 2) why isn't this mentioned in the docs?

And no, it wouldn't be all that complicated--the batch action itself is already a form that's being submitted, and jQuery UI gives us the basic UI functionality. But thanks for the offer @macfanatic :)


Reply to this email directly or view it on GitHub.

@mntdamania
Copy link

I've done something similar by customizing the controller. It looks something like this

  controller do

    def batch_action
      @user = User.new
      if params[:batch_action] == "i_want_a_form"
        render "admin/users/batch_action", :layout => "active_admin"
      else
        super
      end
    end
  end

Here's the gist of backtrace

https://gist.github.com/4417458

When I looked into the source

content_for(:layout)

line yields form as it should on development but on staging its nil

Thanks

@seanlinsley
Copy link
Contributor Author

@macfanatic, here's a work in progress: https://gist.github.com/4485846

Unfortunately Rails UJS doesn't support non-blocking confirm dialogs (see stack overflow and rails/jquery-ujs#283) so I wrote my own dialog handler.

The idea is to return any extra inputs in a batch_action_inputs[] params hash. Ideally it would be associative. I'm looking into the best way to implement that now.

Links for future reference: [jquery-ujs] [AA] [gist 1] [blog post] [gist 2]

@seanlinsley
Copy link
Contributor Author

I've got it mostly working. Here's a snapshot of the UI:

Screenshot

Generated from this syntax:

batch_action :blah, confirm: "Are you sure?", form: {foo: :text, bar: :textarea, baz: :checkbox} do |ids|
  # ...
end

@seanlinsley
Copy link
Contributor Author

While there's no reason why this dialog form couldn't support datepickers, that won't be possible (without a hack) until something along the lines of #1879 is pulled in.

@ghost
Copy link

ghost commented Jan 27, 2013

cool stuff dude

@seanlinsley
Copy link
Contributor Author

@macfanatic any idea how compatible this is with the Bootstrap stuff you're working on?

@macfanatic
Copy link
Contributor

There will be a merge conflict once bootstrapped is in master, but only b/c the JS & CSS assets just need to be relocated in the new structure, so not a lot of work at all. Overall I think it should be easy to integration.

I'm still not sold on this being actually added to AA however.

@seanlinsley
Copy link
Contributor Author

Cool.

Why not? All this is, is a DSL put on top of jQuery UI modal dialogs to allow you to define custom forms wherever you want, along with some integration into the Batch Actions controller. IIRC, we already depend on jQuery UI, so all this PR does is properly utilize the existing toolset.

This really is multipurpose. For example, in the AA app which I developed this for, I used these dynamic forms for Batch Actions, random "confirm with extra input" resource actions, as well as the calendar (click on a day, you can edit/create an event).

Now given it could be prettier, but I'm welcome to suggestions.

@macfanatic
Copy link
Contributor

I think I'd be much more open to the integration if instead of accepting a hash of args, if we could render a full formtastic partial.

On Apr 01, 2013, at 12:13 PM, Sean Ian Linsley notifications@github.com wrote:

Cool.
Why not? All this is, is a DSL put on top of jQuery UI modal dialogs to allow you to define custom forms wherever you want, along with some integration into the Batch Actions controller. IIRC, we already depend on jQuery UI, so all this PR does is properly utilize the existing toolset.
This really is multipurpose. For example, in the AA app which I developed this for, I used these dynamic forms for Batch Actions, random "confirm with extra input" resource actions, as well as the calendar (click on a day, you can edit/create an event).
Now given it could be prettier, but I'm welcome to suggestions.

Reply to this email directly or view it on GitHub.

@seanlinsley
Copy link
Contributor Author

But where would the partial exist before you click on a modal dialog/form? The great thing about this solution is it dynamically generates the form, and throws it away when it's no longer needed.

@macfanatic
Copy link
Contributor

Something like this is more of what I imagined:

batch_action :publish, form: true do |*args|
  # something exciting
end

Or maybe you don't even have to pass form: true, if the partial exists then it is rendered, otherwise it behaves like a normal batch action. Or pass a string to provide a custom partial path to render, like how the sidebar method works.

app/views/admin/posts/batch_actions/_publish.html.haml

form_tag url: batch_action_form_url(resource_class) do |f|
  = f.text_field :attr

Just a thought, I don't think declaring a form via a hash is a maintainable DSL :)

@seanlinsley
Copy link
Contributor Author

What do you mean, "like how the sidebar method works"? What does it do that's special?

The problem with using view partials, is they can't be used for dynamic data on the page. Going back to the calendar use-case, this is all I need to have a dynamic form for both creating new events, and editing existing ones.

$ ->
  # Form submission handler
  $('form#calendar_events').on 'confirm:complete', (e, inputs, parent)->
    $('#event_id').val $(parent).data 'id'
    $('#event_day').val inputs.day
    $('#event_schedule').val $(parent).data('schedule') || inputs.schedule
    $(@).submit()

  # Moving an existing event to a different day
  $('table.calendar li').click ->
    AA.modalDialog "When would you like to move this to?<br>From #{$(@).data 'day'} to..."
      day: 'datepicker'
      (inputs)=>
        $('form#calendar_events').trigger 'confirm:complete', [inputs, @]

  # Creating a new event
  $('#create_calendar_event').click ->
    AA.modalDialog 'Enter the date and schedule for this new event.'
      day: 'datepicker', schedule: ['Foo', 'Bar', 'Baz']
      (inputs)=>
        $('form#calendar_events').trigger 'confirm:complete', inputs

That's two different "form partials", which can be bound to any element in the DOM.

Then the static HTML side of things:

ActiveAdmin.register_page 'Calendar' do
  controller do
    def index
      # initialize the Calendar object
    end
  end

  # Create event or use existing; update appropriate relations in the model logic.
  page_action :events, method: :post do
    # create or update the calendar event, then return a useful message
    redirect_to calendar_path, notice
  end

  action_item do
    a "Create New Event", id: 'create_calendar_event'
  end

  content do
    form_for :event, url: calendar_events_path, html: {style: 'display: none', id: 'calendar_events'} do |f|
      f.text_field :id
      f.text_field :day
      f.text_field :schedule
    end
    # and render the calendar here...
  end
end

@seanlinsley
Copy link
Contributor Author

For the power and flexibility that gives me, that's really not a lot of code.

@seanlinsley
Copy link
Contributor Author

To implement the same thing with proper view partials, I'd have to make rendering happen in an AJAX request.

@seanlinsley
Copy link
Contributor Author

And now with not a little confusion, this PR is rebased on the current upstream master 🔫

@seanlinsley
Copy link
Contributor Author

@chrise86 I explain exactly how to use it at the beginning of this PR :)

@chrise86
Copy link

Just realised the error of my ways, using the wrong branch... :-/ (hangs head in shame...)

@seanlinsley
Copy link
Contributor Author

Ah, no worries 🐈

As you can see I didn't do a great job with the look & feel of the popup, but at least it works. Changes are welcome.

@chrise86
Copy link

chrise86 commented May 1, 2013

What about this?

Modal styling

Submitted a pull request if you're interested :)

@chrise86
Copy link

chrise86 commented May 1, 2013

FYI when using this branch / fork of activeadmin the menu items are moving around randomly with each page load, and parent / child items do not always appear...

Switching to v0.6.0 on gregs main repo fixes this

@seanlinsley
Copy link
Contributor Author

Thanks for the contrib @chrise86 💚

@@ -99,17 +102,31 @@ class BatchAction
# => You can create batch actions with a title instead of a Symbol
#
# BatchAction.new( :flag, :if => proc { can? :flag, AdminUser } ) { |selection| }
# => You can provide an optional :if proc to optionally display the batch action
# => You can provide an optional `:if` proc to optionally display the batch action
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrite this to:

You can provide an :if proc to choose when the batch action should be displayed

@seanlinsley
Copy link
Contributor Author

Thanks for the vote of confidence @anthonylebrun. I'll work on updating this PR to the latest code on master.

@anthonylebrun
Copy link
Contributor

@daxter Awesome! I'll be watching this thread with bated breath.

@seanlinsley
Copy link
Contributor Author

Okay, this has been updated to master. Now to write tests and documentation.

@anthonylebrun
Copy link
Contributor

@daxter , you are the man.

@include section-header;
span { font-size: 1.1em; }
}
.ui-dialog-titlebar-close { visibility: hidden }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, needs to be display: none

@seanlinsley
Copy link
Contributor Author

Okay, just rebased this for hopefully the final time.

There's a problem with the default destroy batch action:
screen shot 2013-11-11 at 11 49 36 am

I don't like that it wraps on two lines like that. Maybe we should just use the native JS confirm prompt when there aren't any form inputs to fill out?

thanks to @chrise86 for the updated CSS so this isn't horribly ugly
+ rename from `modalDialog` to `modal_dialog`
+ use the default confirm message on the form if none given
@seanlinsley
Copy link
Contributor Author

  • add tests (waiting on Get JS test suite running #2727)
  • find some way to translate the modal form itself
  • find a better solution to align multiple inputs so this doesn't happen:

screen shot 2013-11-23 at 3 22 09 pm

seanlinsley added a commit that referenced this pull request Nov 23, 2013
@seanlinsley seanlinsley merged commit 6c2dc9c into activeadmin:master Nov 23, 2013
@seanlinsley seanlinsley deleted the 1815-batch-action-forms branch November 23, 2013 21:24
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling cb95ebc on seanlinsley:1815-batch-action-forms into 56e93bb on gregbell:master.

@mindhalt
Copy link
Contributor

As for alignment... Maybe it will be more appropriate to place label above input field? This way problem with alignment will disappear ^_^

@seanlinsley
Copy link
Contributor Author

Yeah that'd be an option. I think when I first implemented this I set a static width on the labels, but later on I took it out because I wanted a wider label than the width I had originally set.

@jdurand
Copy link

jdurand commented Feb 9, 2014

That's great stuff @seanlinsley ! 👍

@seanlinsley
Copy link
Contributor Author

Glad you like it 🐱

@anthonylebrun
Copy link
Contributor

Woohoo, thanks @seanlinsley . Can't wait to try it on my next project!

@Fivell
Copy link
Member

Fivell commented Oct 19, 2014

Good job! However I didn't find ability to access request data from form block.
is it possible to do something like

  batch_action :move_to, form: ->{
    {
        group_id: User.find(params[:user_id]).groups.pluck(:name, :id)
    }
  } do |ids, inputs|
    Items.where(id: ids).update_all(group_id: inputs[:group_id])
    redirect_to :back
  end

?

@timoschilling
Copy link
Member

@Fivell your example should work, if not inspect inputs

@Fivell
Copy link
Member

Fivell commented Oct 20, 2014

@timoschilling , this doesn't work because I think proc is evaluated in wrong scope
group_id: User.find(params[:user_id]).groups.pluck(:name, :id) in this line a have
undefined local variable or method params' for #ActiveAdmin::ResourceDSL:0x007f9949ceb348`

using master branch

one more thing is that this resource is using belongs_to :user
In this example retrieving of groups only for particular user is needed .

inside proc self is <ActiveAdmin::Resource::BelongsTo:0x007f9949ceada8 @owner=#<ActiveAdmin::Resource:0x007f994c113e00 ...>, @target_name=:......

@seanlinsley
Copy link
Contributor Author

That's because the proc isn't evaluated in the view context, it's simply called as-is. If you'd like that feature, please submit a PR.

@timoschilling
Copy link
Member

I'm already working on that

@modsaid
Copy link

modsaid commented Feb 9, 2015

This optional :form attribute would be very useful in page_action or even action_item too

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.

None yet