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

Use page refreshes #4

Draft
wants to merge 2 commits into
base: initial
Choose a base branch
from
Draft

Use page refreshes #4

wants to merge 2 commits into from

Conversation

jorgemanrubia
Copy link
Member

No description provided.

@northeastprince
Copy link

northeastprince commented Nov 27, 2023

@jorgemanrubia is the refresh broadcast working for you? It wasn't for me, which is why I opened hotwired/turbo-rails#522.

@@ -1,5 +1,5 @@
class Task < ApplicationRecord
belongs_to :project
belongs_to :project, touch: true

Choose a reason for hiding this comment

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

does Hey really use callbacks/touch like this in real apps, or is it only for the sake of example?

Choose a reason for hiding this comment

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

Yeah, they most likely do, since it's DRY and allows you to quickly invalidate caches as well, which they use heavily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I can confirm we do!

@jorgemanrubia
Copy link
Member Author

@northeastprince it's working indeed. Do you mean that it's not working even if you clone this repo? Or in your app? If the later, which Rails version are you using?

@northeastprince
Copy link

@jorgemanrubia I shared the issue here, my app is running on Rails edge. Let me know if you need more info!

@@ -9,6 +9,9 @@
<%= stylesheet_link_tag "application", "data-turbo-track": "reload" %>
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bulma@0.9.4/css/bulma.min.css">
<%= javascript_importmap_tags %>

<%= turbo_refreshes_with method: :morph, scroll: :preserve %>
Copy link

@bradgessler bradgessler Nov 28, 2023

Choose a reason for hiding this comment

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

I'm experimenting with Turbo 8 via Phlex components and was surprised to see that the meta tags all use the provide :head construct instead of just emitting a tag.

If you want to make the tags accessible from outside of the content view, you might either eliminate the provide :head construct or keep that and expand out the methods like this:

  # Pages that are more likely than not to be a cache miss can skip turbo cache to avoid visual jitter.
  # Cannot be used along with +turbo_exempts_page_from_preview+.
  def turbo_exempts_page_from_cache
    provide :head, turbo_exempts_page_from_cache_tag
  end

  def turbo_exempts_page_from_cache_tag
    tag.meta(name: "turbo-cache-control", content: "no-cache")
  end

I think the reason you want to keep the provide :head tag is because the meta tags within each view file needs to put the meta tag in the head? The approach I provided above should serve both needs.

Choose a reason for hiding this comment

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

Here's what I ended up doing in my Phlex layout:

        # Turbo 8 has the helper method `turbo_refreshes_with method: :morph, scroll: :preserve` that
        # inserts the content in the `head` tag. Since Phlex doesn't work that way, we'll just put the tags here.
        meta(name: "turbo-refresh-method", content: "morph")
        meta(name: "turbo-refresh-scroll", content: "preserve")

Not a huge deal, but it would be nice to go through the logic that checks for argument validity if the tags are broken apart.

Choose a reason for hiding this comment

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

@bradgessler from Chicago / Poll Everywhere. Funny running into you on a random PR comment thread. :)

Choose a reason for hiding this comment

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

@krschacht speaking to you live from CA, and running into similar ergonomic issues that you're running into.

Choose a reason for hiding this comment

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

I opened a PR at hotwired/turbo-rails#542 that decouples tag generation from the way it's served. We can continue this discussion over there.

@@ -1,3 +1,7 @@
<% @projects.each do |project| %>
<%= turbo_stream_from project %>
<% end %>

Choose a reason for hiding this comment

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

@jorgemanrubia When I open the projects index (http://localhost:3000/projects) in two separate browser windows, and in one of them I create a new project, the other one does not update with the new project. I have to manually refresh the page to see it.

I assume it's because of this block here ^, it only subscribes to the specific projects which exist at the time the page loads so it's not subscribed to a newly created project. One solution would be to introduce a new higher-level abstraction called Portfolio (a collection of projects) and broadcast refreshes from that. But that seems heavy-handed if we don't otherwise need a new model.

Is there a simpler solution to this within the Turbo 8 Morphing world?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Check this PR hotwired/turbo-rails#521. You can use the pluralized model name as the stream name for listening to additions.

Choose a reason for hiding this comment

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

But would this also work when we have scopes in place? For example like:

  <%= turbo_stream_from @projects.not_deleted.is_active.where(client_id: 1) %>

Or would it just subscribe to all the projects?

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.

6 participants