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

Turbo-frame request variant #229

Open
tleish opened this issue Sep 8, 2021 · 7 comments
Open

Turbo-frame request variant #229

tleish opened this issue Sep 8, 2021 · 7 comments

Comments

@tleish
Copy link

tleish commented Sep 8, 2021

Any interest in adding a turbo-frame request variant to Turbo::Frames::FrameRequest?

# turbo-rails/app/controllers/turbo/frames/frame_request.rb

module Turbo::Frames::FrameRequest
  extend ActiveSupport::Concern

  included do
    layout -> { false if turbo_frame_request? }
    etag { :frame if turbo_frame_request? }
+   before_action :turbo_frame_request_variant
  end

  private
    def turbo_frame_request?
      request.headers["Turbo-Frame"].present?
    end

+   def turbo_frame_request_variant
+     request.variant = :turbo_frame if turbo_frame_request?
+   end
end

This provides the option to change behavior on controller actions for turbo-frame requests:

class PostsController < ApplicationController

  #  if turbo_frame_request? && new.html+turbo_frame.erb exists
  #    renders posts/new.html+turbo_frame.erb
  # else
  #    renders posts/new.html.erb
  # end
  def new
    @post = Post.new
  end

  # render different partial for turbo_frame
  def edit
    @post = Post.new
    respond_to do |format|
      format.html do |variant|
        variant.turbo_frame { render partial: 'posts/turbo_frame_form' } 
      end
    end
  end

  # turbo-frame variant behavior
  def create
    respond_to do |format|
      format.turbo_stream do |variant|
        variant.turbo_frame { redirect_to ... } # turbo-frame variant redirect
        variant.none { redirect_to ... } # default redirect
      end
      format.html { redirect_to ... }
    end
  end
end

see also: hotwired/turbo#378

seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Sep 17, 2021
Closes hotwired#229

---

Mark the request's [variant][] as `:turbo_frame` whenever the
`Turbo-Frame:` header is present in the request. Also adds documentation
to describe how to achieve the previously built-in layout optimizations.

[variant]: https://guides.rubyonrails.org/4_1_release_notes.html#action-pack-variants
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Sep 17, 2021
Closes hotwired#229

---

Mark the request's [variant][] as `:turbo_frame` whenever the
`Turbo-Frame:` header is present in the request. Also adds documentation
to describe how to achieve the previously built-in layout optimizations.

[variant]: https://guides.rubyonrails.org/4_1_release_notes.html#action-pack-variants
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Sep 25, 2021
Implements `Request#turbo_frame?` predicate along with the
`Request#turbo_frame` accessor. Both read from the `Turbo-Frame:`
header.

Mark variant as `turbo_frame` when header present
---

Closes hotwired#229

Mark the request's [variant][] as `:turbo_frame` whenever the
`Turbo-Frame:` header is present in the request. Also adds documentation
to describe how to achieve the previously built-in layout optimizations.

[variant]: https://guides.rubyonrails.org/4_1_release_notes.html#action-pack-variants
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Sep 25, 2021
Implements `Request#turbo_frame?` predicate along with the
`Request#turbo_frame` accessor. Both read from the `Turbo-Frame:`
header.

Mark variant as `turbo_frame` when header present
---

Closes hotwired#229

Mark the request's [variant][] as `:turbo_frame` whenever the
`Turbo-Frame:` header is present in the request. Also adds documentation
to describe how to achieve the previously built-in layout optimizations.

[variant]: https://guides.rubyonrails.org/4_1_release_notes.html#action-pack-variants
@WriterZephos
Copy link

WriterZephos commented Oct 31, 2021

I strongly recommend doing this. It doesn't make a lot of sense to not have an easy way to respond differently to from requests coming externally, and requests coming from internal links. For example, if someone types in a url to the show view of some model, you want to render with the full application layout, but if you are responding in the controller to render only part of the page using turbo-frames, they would only see that.

Since I don't like having a bunch of different views that only differ in their file extensions and other minor things, I like to render the same view and use different templates for replacing turbo frames in the dom or rendering the full page. This solution works great to accomplish that.

@WriterZephos
Copy link

Just wanted to drop a link to the gem I am working on that would benefit from this change. The module here dynamically selects a layout based on the request variant:

https://github.com/WriterZephos/TurboRouter/blob/main/lib/turbo_router/controller.rbz

I am rebuilding the above repo using rails plugin new instead of bundle gem but the demo app that gets generated for testing doesn't have hotwire installed, so I am waiting for that to be fixed.

@SleeplessByte
Copy link

Even though hotwired/turbo#378 (comment) says it shouldn't, this really should have been solved by registering a new media type, which has the added benefit of working with non-rails solutions, is controllable from the content, and allows you to push the turbo frame as a media type argument.

I do a lot with media types and content negotiations and in my [experienced] opinion tleish isn't correct. The format of the content is indeed HTML, but it's the same issue of application/json (syntax) vs application/vnd.x.y.z+json which says syntax is json, but the type is described more specifically.

In the same vain, a turbo-frame response actually needs to conform to specific properties (it should contain the same frame) and thus a media type that is an html type is actually appropiate.

The API would then be:

format.html do
  # full document response
end
format.turbo_frame do 
  # turbo frame response
end
format.turbo_stream do
  # turbo stream response
end

The proposed Accept header for the turbo library would be:

Accept: text/vnd.turbo-frame.html; id=<frame_id>, text/vnd.turbo-stream.html, text/html, application/xhtml+xml

Order of accepting is then:

  • text/vnd.turbo-frame.html; id=<frame_id>
  • text/vnd.turbo-stream.html
  • text/html
  • application/xhtml+xml

So this also means that if you do not want to return a turbo frame:

format.html do
  # full document response
end
format.turbo_frame do 
  # turbo frame response
end

...and this also means that if the turbo library detects it's not "frameable", it can send:

Accept: text/vnd.turbo-stream.html, text/html, application/xhtml+xml

...and you then do not need to do #278, because Accept should be in the Vary already.

Using a custom header (Turbo-Frame) and then using rails specific extra negotiation on the server (variants) makes it harder to interopt with this in other systems.

@tleish
Copy link
Author

tleish commented Aug 18, 2022

FYI, @dhh does not like the idea of using variants for turbo-frames

I don't love the idea of using variants for turbo frames. We did variants originally to provide different types of HTML responses for different platforms, like a native iOS app. Where what you want to send is something substantially different because of the constraints of the platform. I don't think that applies here.

I'm fine with having a way to detect turbo frame requests as a conditional, but I think that's as far as it should go. This should be the exception. If you're branching all your responses for frames vs not, something isn't right, and we should investigate those pressures in a different way.

see: #250 (comment)

@SleeplessByte
Copy link

Thanks. I had not seen that (yet), but matches how I think about it.

Unfortunately I doubt turbo frames will be turned into a media type.

@tonywok
Copy link

tonywok commented Mar 16, 2023

Edit: Minimizing this as I agree, in hindsight, perhaps a bit off-topic. Initially I thought contributing a use-case might contribute to this issue's discussion, but I see it's a bit different than how I had initially interpreted.

Curious to hear what you folks think about this use case... I think I agree with DHH that this feels distinct from usage of media type, fwiw.

I have a GET-form that does a bunch of filtering. I use turbo advance so that folks can share the url and see the filtered results. The response is pretty hefty, so I want to show some kind of loading indicator.

A typical implementation of a turbo frame that lazily sources the frame contents from another controller solves the problem when visiting this page from somewhere else in the app. The contents of the lazy frame can show a loading indicator, and get swapped out. However, afaict, this leaves me with the following issues:

  1. Upon re-submission of the form (e.g modifying a filter), I don't see the loading indicator (because I'm in the new frame that is not lazy)
  2. After advancing the url due to submission, if I were to browser refresh the page, I don't see the spinner because the controller that fields the response returns a non-lazy frame.

I can resolve (1) by using some CSS to hide/show the content and spinner based on the presence of the busy attribute on the turbo frame tag.

I can resolve (2) by using controller.turbo_frame_request?, conditionally making the frame lazy if it's not originating from a turbo frame request -- making it non-lazy if it is originating from a turbo frame request.

@SleeplessByte

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants