hotwired / turbo-rails

Use Turbo in your Ruby on Rails app
https://turbo.hotwired.dev
MIT License
2.11k stars 328 forks source link

Layout not omitted for turbo frames when using custom layout #268

Closed morgoth closed 1 year ago

morgoth commented 2 years ago

Let's say having:

AdminController < ApplicationController
  layout :admin
end

When I use turbo frame request in sample controller:

UsersController < AdminController
  def show
  end
end

The optimization to omit layout in https://github.com/hotwired/turbo-rails/blob/a0f14b9e4649f60bcf8fa8b82f35e7f460d60177/app/controllers/turbo/frames/frame_request.rb#L16 does not work.

I can see that this bug is known, ie https://github.com/hotwired/turbo-rails/pull/232#issuecomment-930268246 but opening this issue to keep track on this specific problem.

WriterZephos commented 2 years ago

I don't consider this a bug. You want dynamic behavior but you are setting the layout in a static way. Try setting your layout with a proc instead of a static value.

The change you are asking for would break other people's code (including my own) where we rely on being able to set the layout and override the turbo default.

HalfBloody commented 2 years ago

This behaviour breaks the turbo frame silently when it's placed in the layout because the header already contains an empty turbo frame with the same name.

image

It's counter-intuitive that you can place a turbo_frame in Application Layout and it will work but if you place it in custom layout it will unexpectedly not work. This will hinder Turbo adoption.

samlachance commented 2 years ago

I ran into something similar to what @HalfBloody did. In my case, though, I was using a turbo-frame with a src which errored when the returned HTML included the layout, which included the turbo-frame with a src

I'm not sure I understand why there is or should be a connection between whether a layout is set and whether a layout will be included in the turbo-frames response. Different layouts are used for all sorts of reasons.

I feel like the current behavior is only intuitive and desirable if you are employing a very specific design pattern. It sounds like the current behavior could be utilized in interesting ways but I'm not sure it's worth not being able to use turboframes in a layout if a layout is set in a controller.

nickjj commented 1 year ago

I've encountered this too. It's kind of a big deal because it means our targeted frames are really executing all of the queries associated with other areas of the layout.

I tried setting the layout in 2 different ways:

class Courses::ApplicationController < ApplicationController
  # layout 'courses'
  layout Proc.new { 'courses' }
end

In both cases the full page's content (from <html> to </html>) gets sent in the response when navigating links within a frame.

Does anyone have a suggested workaround?

For more context my custom courses layout has:

<% content_for :content do %>
  <%= render 'courses/video' %>

  <div>
    <%= turbo_frame_tag 'course_details', data: { turbo_action: 'advance' } do %>
      <%= render 'courses/nav' %>
      <%= yield %>
    <% end %>
  </div>

  <%= render 'courses/toc' %>
<% end %>

<%= render template: 'layouts/application' %>

The concern is rendering the video and toc is pretty expensive. There's a bunch of queries that get run and none of that content needs to change when navigating the links in the course detail's frame.

morgoth commented 1 year ago

@nickjj You can write in the given controller action render layout: !turbo_frame_request?

nickjj commented 1 year ago

@morgoth that ends up bypassing the frame and makes links transition in the same way as Turbo Drive. That was pretty unexpected behavior. I am using nested frames too. Basically that "courses_details" frame has other frames that are used within it.

tmaier commented 1 year ago

I was also hit by this, but the other way around. I did not know that turbo-rails would set the layout to false automatically. I started to write layout: false in many related renders (e.g. of modals)

I would vote for properly documenting this behaviour.

The solution is to recommend to not use layout :admin in the controller, where admin is the name of the layout file, but to set it to a method.

For example

AdminController < ApplicationController
  layout :determine_layout

  private

  def determine_layout
    return false if turbo_frame_request?

    :admin
  end
end
cice commented 1 year ago

We have just been bitten by this hard, too. Imho this should be documented

nickjj commented 1 year ago

What pattern would you use to get this to work when the following conditions are true:

  1. You have a custom layout
  2. Your custom layout includes defining a turbo_frame_tag
  3. You have a bunch of view partials that set a turbo_frame target to the frame defined in step 2

With the documentation's suggested changes, I do only get back the turbo_rails/frame.html.erb response but I also get a "Content missing" error and it's due to the response not containing the expected frame defined in step 2.

For example, here's my custom layout in layouts/course/player.html.erb:

<% content_for :content do %>
  <main>
    <div class="min-h-screen md:flex">
      <div class="flex-1">
        <%= turbo_frame_tag "course_video_player" do %>
          ...
        <% end %>

        <%= turbo_frame_tag "course_details", data: { turbo_action: "advance" } do %>
          <%= render "courses/nav" %>
          ...
        <% end %>
      </div>

      <%= render "courses/toc" %>
    </div>
  </main>
<% end %>

<%= render template: "layouts/application" %>

Then inside one of my views for this layout I have a partial with:

<%= link_to "Hello world", "...", data: { turbo_frame: "course_details" } %>

When I click that link, that's when I get the content missing error.

I think this makes sense because the course_details frame doesn't exist since I made a frame request which means the turbo_rails/frame.html.erb layout is being used which doesn't have that frame defined. But I'm not quite sure what an ideal pattern looks like here to make this work. Especially since the layout has a lot of other common things, having to duplicate essentially the entire course player's layout in each view which would be painful. The "real" version has like 100 lines of HTML and I have a lot of views.

Ideally that frame and its associated layout related HTML would exist once somewhere, which is what I did back when the layout loaded in each response but now the layout isn't loaded for frame requests (which technically is the result I want, except I still want the benefits of the layout). What am I doing wrong?

equivalent commented 3 months ago

let me throw one more experience into the pile for googling folks:

company I work for is doing some redesign of entire layout managed by Flipper feature flags. So we keep the original design under "application" layout and new redesign in "application_new"

So In old RoR days I would do

class ApplicationController < ActionController::Base
  layout "application_new" if Flipper.enabled?(:new_design)
  #...
end

but this caused turbo frames to stop working on the new layout yet continue to work on application.html.erb layout.

After hours of debugging I found this thread and the solution should be:

class ApplicationController < ActionController::Base
  layout :determine_layout

  def determine_layout
    return false if turbo_frame_request?
    return "application_new" if  if Flipper.enabled?(:new_design)
    nil # fallback to default Rails layout, in this case "application"
  end
end

(reasons explained in comments above 👆)