Works on success, but not on validation errors

artemave commented 9 years ago

Here is the form:

= simple_form_for @proposal, turboboost: true do |f|
  = f.error_notification

  .form-inputs
    = f.input :title
    = f.input :description

  .form-actions
    = f.button :submit

and the controller:

  def create
    @proposal = CreateProposalService.call(params: proposal_params, user: current_user)

    if @proposal.persisted?
      redirect_to @proposal, notice: 'Proposal was successfully created.'
    else
      render :new
    end
  end

The "success" branch works as expected, but the "fail" branch does not replace the body with the result of render :new. I can see it in the response, but that is about as far as it gets.

What am I missing?

waymondo commented 9 years ago

If you want to render the errors for the "fail", you need to trigger the exceptions with something like @proposal.save! or use render_turboboost_errors_for(@proposal). https://github.com/waymondo/turboboost#error-handling

You'd also need to enable the JS for automatic error insertion if that is what you are expecting: https://github.com/waymondo/turboboost#automatic-error-message-insertion

Or if you want to render the :new action again, you need to specify the DOM element and render option (i.e. render :new, within: '#container'). https://github.com/waymondo/turboboost#scoped-response-rendering

Or you can use redirect_to new_proposal_url with a flash message to get a similar result.

Give the README a read through and let me know if that clears things up.

I'm not leaning towards rendering the action of :new into the body as the default behavior currently because the JS format version of render doesn't contain the layout. As such, the yield containing element is unknown and this would break the layout. If you have a different idea of how it should work, my ears are open.

artemave commented 9 years ago

One important premise of turbolinks is that the same code works seamlessly without javascript. I was expecting turboboost to adhere to this principle since it aims to be a turbolinks extension.

As for my example in particular, I am still confused as to why render :new works with turbolinks, but does not work with turboboost. No errors are thrown, it just does not work. Looks like a broken behavior.

I'm not leaning towards rendering the action of :new into the body as the default behavior currently because the JS format version of render doesn't contain the layout.

That is the key there, turbolinks does not operate in JS format. And that is exactly what I'd expect from turboboost.

waymondo commented 9 years ago

What are you proposing as the default behavior then, rendering the action within: 'body' if its not specified? That in turn assumes the developer is strictly using layouts where their views are fully yield-ed at the root of <body>, which is often not the case.

If that is your case and you specify render :new, within: 'body' and it would work seamlessly with and without JavaScript.

When you render format html, the layout is re-rendered as well which adds additional server overhead and response size. Yes, this is how turbolinks works and is something that I am trying to optimize away with this gem. This is the primary reason behind my personal preference of only serving back raised exception with a reactive error handler, instead of re-rending the form at all.

I'm still open to ideas here, but I'm not seeing altering the default behavior yet. If you don't like this approach, perhaps you'll prefer turbolinks 3.0+'s method of implementing this same feature. Their optimized approach involves additional render options as well though.

artemave commented 9 years ago

render :new, within: 'body' works, but then submitting the form after it's been rendered with errors is doing full page reload.

waymondo commented 9 years ago

Thanks, I am seeing this. Can you open a new issue for this bug? I will work on a fix.

waymondo commented 9 years ago

One thing I'm noticing is that if you wrap the rendering in a respond_to block for both html and JS, this goes away:

respond_to do |format|
  format.html { render :new }
  format.js { render :new, within: 'body' }
end

I will look at it more though.

artemave commented 9 years ago

format.js { render :new, within: 'body' } renders without layout

waymondo commented 9 years ago

Yes, this is true. I was basing format.js { render :new, within: 'body' } based off this implication:

What are you proposing as the default behavior then, rendering the action within: 'body' if its not specified? That in turn assumes the developer is strictly using layouts where their views are fully yield-ed at the root of body, which is often not the case.

If your target container for the :new action is different than the body, you would need to specify it. Or use something like render template: 'new' or render partial: 'form'.

waymondo commented 9 years ago

^ the above commit will address the issue where the 2nd form submission does a full-page refresh when the action is defined as the following:

if @proposal.persisted?
  redirect_to @proposal, notice: 'Proposal was successfully created.'
else
  render :new, within: 'body'
end

waymondo / turboboost

Works on success, but not on validation errors #12