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 target node option for html async form response #274

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulcookie
Copy link

so it will insert response html to target node witch gets as jquery selector from form "data-target" if "data-type" is "html"

for example:

app/views/contact_message/new

#form_container
  = render 'form', :contact_message => @contact_message

app/views/contact_message/_form

= form_for contact_message, :remote => true, :html => {:data => {:type => :html, :target => '#form_container'}} do |f|
  = f.text_field :message
  = f.submit 'Submit'

app/controllers/contact_message_controller.rb

class ContactMessageController < ApplicationController

  def new
    @contact_message = ContactMessage.new
  end

  def create
    @contact_message = ContactMessage.new(params[:contact_message])

    if @contact_message.save
      @contact_message = ContactMessage.new
      flash.now.notice = 'success'
    else
      flash.now.alert = 'some errors'
    end

    if request.xhr?
      render :partial => '/contact_message/form', :locals => {:contact_message => @contact_message}
    else
      render :new
    end
  end

end

@JangoSteve
Copy link
Member

I've done something like this for some of my older projects, to upgrade to rails 3, using something like this in my application js:

$j(document).on('ajax:complete', 'a[data-remote][data-update], form[data-remote][data-update], select[data-update]', function(e, data) {
  var update = $j(this).data('update');
  $j(update).html(data.responseText);
});

I'm not 100% sure we want it in jquery-ujs, since it's really easy to do with the tools provided (and I think this functionality is provided in the jrails gem).

That being said, I do like the fact it's only a couple lines of code to include. If we were to include it, does it make sense to only include it in the ajax:success callback? Might we also want it in the ajax:error callback? Or in that case, I'd just put it once in the ajax:complete callback, so it gets called either way.

@bcm
Copy link

bcm commented Jan 19, 2013

this approach assumes that the response data is exactly the html string you want to stuff inside the target element. but what if the response data includes multiple items that will be used by various event handlers? how would this builtin handler know how to extract the right item? and what if you really want to replace the node rather than setting its contents?

as @JangoSteve indicated, this is extremely simple functionality to add anywhere you need it in your application, but I don't immediately see a way to do it in this library in a way that would satisfy everyone without a lot of overcomplicated markup to configure the behavior for each individual's specific needs.

@JangoSteve
Copy link
Member

@bcm good points. I'm going to play devil's advocate and just say, that if we did include this, it'd likely be in a way that mimics the update attribute from the rails days before UJS, so I'd think those other use-cases, you'd just pick a different data attribute (e.g. data-my-response) and implement yourself. If we did implement this, it'd just be for the one most-common simplest use-case.

But again, I'm not necessarily saying I think this should be implemented, just trying to see what discussion comes about.

@bcm
Copy link

bcm commented Jan 19, 2013

I actually like the original patch's approach:

if (dataType == 'html' && target) { $(target).html(data); }

that's less ambiguous. if the server sends back raw html it probably wants it to be inserted into the document somewhere.

if you went that route, you might also support data-replace as well as data-update.

@paulcookie
Copy link
Author

I agree. And what solution we choose?

@JangoSteve
Copy link
Member

Just realized, we never decided on the approach for this. I'd say keep it as is, but move the HTML insertion from the success callback to the complete callback instead.

@JangoSteve
Copy link
Member

Also, if you could write a test for this, that'd be even better and I could pull it in right away. Otherwise, I'll have to write one when I can get to it.

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