Separate turbo meta tag generation from provides #542

bradgessler · 2023-12-13T19:25:25Z

Removes the requirement to have a content_for :head tag in the head of the Rails application. Instead the developer just adds the turbo_meta_tags(method: :morph, scroll: :preserve) to the top of their pages and it will setup the yield :turbo_head block that the helpers provide via provide :turbo_head.

That means developers can do this:

<head>
  <%= turbo_meta_tags method: :morph, scroll: :preserve %>
</head>

Instead of this:

<head>
  <%= turbo_refreshes_with method: :morph, scroll: :preserve  %>
  <%= content_for :head %>
</head>

And prevent confusion like this: https://www.reddit.com/r/rails/comments/186z9lu/comment/kd60oq4/?utm_source=reddit&utm_medium=web2x&context=3

This also makes it possible to use the meta tag helpers from outside Rails rendering, such as an app built entirely from Phlex components.

If there's general interest in merging PRs that improve the developer experience, I could make additional improvements that make configuring Turbo pages easier from the controller in addition to the view.

jorgemanrubia · 2023-12-13T20:15:57Z

Hey @bradgessler,

Thanks for your work here. I don't love the breaking API change here. Furthermore, a nice thing with the current approach is that you can use helpers like turbo_refreshes_with and turbo_exempts_page_from_cache from regular view templates, not necessarily in the header. For example, you might place them at the top of show.html.erb in a controller action, and you know those will land in the <head> section.

Maybe an alternative could be to just provide _tag alternatives so that you can use either. E.g: turbo_refreshes_with_tags(...) or turbo_exempts_page_from_cache_tag(...), so that you can use those without having to use Rails' layouts?

bradgessler · 2023-12-13T20:43:08Z

Thanks for your work here. I don't love the breaking API change here. Furthermore, a nice thing with the current approach is that you can use helpers like turbo_refreshes_with and turbo_exempts_page_from_cache from regular view templates, not necessarily in the header. For example, you might place them at the top of show.html.erb in a controller action, and you know those will land in the section.

The changes shouldn't be breaking any APIs or change the behavior of what's already there, with the exception of providing content for :turbo_head instead of :head.

This PR only adds to the API with helpers like turbo_meta_tags. Everything else should continue functioning as it already was, including displaying tags in the <head/> and in the body of the HTML document. It also adds various *_tag helpers that you're recommending in your response that will work outside of Rails.

jorgemanrubia · 2023-12-13T21:03:36Z

The changes shouldn't be breaking any APIs or change the behavior of what's already there, with the exception of providing content for :turbo_head instead of :head.

That's what I mean by breaking change. It will break existing usages if they don't change the code after updating.

I'd just leave the _tag variants in the PR and remove the turbo_meta_tags helper and layout changes. I don't think they bring a significant improvement over what we have, or that they read better, so I'd rather don't change that.

bradgessler · 2023-12-14T01:21:57Z

Ok I switched the :turbo_head block back to :head (I didn't realize that was part of the way previous version of Turbo worked).

I disagree about readability of turbo_meta_tags, which is why I provided a link to the Reddit comment of the person asking what the yield :head was all about. The precedent or all the other Rails tag helpers that don't require the yield :head block. Additionally, it's not clear that the Turbo helpers append content into :head.

I do think there's a better way to handle all of this because, as it stands, if a developer calls any of these helpers multiple times, it will keep appending the meta tag to the :head block. An object that could be configured by the views that's yielded to the content :head block would be ideal. I'm pretty sure it could also be done in a way that doesn't break existing APIs. If that's something you'd consider merging I could open a different PR for it; otherwise let's finalize whatever needs to be done here and get it merged.

jorgemanrubia

Thanks for addressing the changes @bradgessler. I think the turbo_meta_tags helper method makes sense to just encapsulate yield :head (without additional options). But I in the PR you removed it completely, despite of it being referred to. I think that's an oversight?

I think it's a good idea to keep it, but without options. So it's just a replacement for yield :head you can use, but if your app is doing yield :head it still works.

We need to review the code docs a bit here, but that was also pending for the new morphing helpers, so we'll address that separately.

bradgessler · 2023-12-19T00:09:02Z

I think it's a good idea to keep it, but without options. So it's just a replacement for yield :head you can use, but if your app is doing yield :head it still works.

Oh I misunderstood ... I thought you requested it be completely removed. Yes! I will add it back without accepting options.

This makes it possible to use the meta tag helpers from outside Rails rendering, such as an app built entirely from Phlex components. The `turbo_meta_tags` method was added to minimize confusion over the seemingly random `content_for :head` that has been observed in early Turbo 8 betas. This is a thin wrapper around `content_for :head`.

bradgessler · 2023-12-19T19:40:09Z

Ok I pushed up the turbo_meta_tags helper method updates that acts as a wrapper around content_for :head. I think that's everything?

jorgemanrubia

Thanks @bradgessler

dhh · 2023-12-26T18:13:10Z

I'm not a fan of turbo_meta_tags, since it'll pull in anything that was content_for :head. Let's pull that out.

Also, this PR is missing documentation for the new tag methods. And the documentation around "These helpers require a +yield :head+ provision in the layout" is now also outdated since we have the new _tag variants. Need to properly describe the difference, the point about using the tag-less helpers inside templates that live separately from the layout, etc.

I think as it stands here, we've made things somewhat worse than they were. So let's get things back on track with proper documentation.

jorgemanrubia · 2023-12-26T18:44:05Z

@bradgessler could you create a PR reverting the turbo_meta_tags helper method and document the tag methods please? If not, I will.

What I don't love about yield :head is that it looks as if it was exposing an implementation detail. You can't tell it's related to turbo when you see it. That was my reasoning, but I get your point about the confusion it can cause. Maybe if the section was called turbo_head we could minimize the odds of unwanted side effects, but that would be a breaking change, sadly.

dhh · 2023-12-26T18:46:22Z

I'd rather push yield :head forward as something all Rails apps have and that you should be able to depend on. Although I wouldn't mind turbo_head either, with a wrapper around, but yes, that's a breaking change, so it's a no go.

dhh · 2023-12-27T16:23:54Z

Actually something else we could do is build a fail safe. If you add content to a variable that's then never yielded, we'd throw a warning. But might be too much overhead/complexity to track that for something so relatively uncommon.

See #542 (comment)

See hotwired/turbo-rails#542 (comment)

bradgessler mentioned this pull request Dec 13, 2023

Use page refreshes basecamp/turbo-8-morphing-demo#4

Draft

bradgessler force-pushed the meta-tags branch 2 times, most recently from 08bf75d to 0c7fabb Compare December 14, 2023 00:45

bradgessler force-pushed the meta-tags branch from 0c7fabb to 823b14f Compare December 14, 2023 17:08

jorgemanrubia reviewed Dec 18, 2023

View reviewed changes

bradgessler force-pushed the meta-tags branch from 823b14f to d966ddf Compare December 19, 2023 19:38

jorgemanrubia approved these changes Dec 24, 2023

View reviewed changes

jorgemanrubia merged commit d1b4201 into hotwired:main Dec 24, 2023

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

f776e22

See #542 (comment)

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

683cd0f

See #542 (comment)

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

39b1d32

See #542 (comment)

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

2f3cfb1

See #542 (comment)

jorgemanrubia mentioned this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation #548

Merged

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

99ed0a3

See #542 (comment)

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

b7de608

See #542 (comment)

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

bc59112

See #542 (comment)

jorgemanrubia added a commit that referenced this pull request Dec 29, 2023

Remove turbo_meta_tags and add missing documentation

6d11e2b

See #542 (comment)

dhh pushed a commit that referenced this pull request Jan 1, 2024

Remove turbo_meta_tags and add missing documentation (#548)

0f00575

See #542 (comment)

luisjose1996 added a commit to luisjose1996/Turbo-Rails that referenced this pull request May 10, 2024

Remove turbo_meta_tags and add missing documentation (#548)

938cc8b

See hotwired/turbo-rails#542 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate turbo meta tag generation from provides #542

Separate turbo meta tag generation from provides #542

bradgessler commented Dec 13, 2023 •

edited

Loading

jorgemanrubia commented Dec 13, 2023

bradgessler commented Dec 13, 2023

jorgemanrubia commented Dec 13, 2023

bradgessler commented Dec 14, 2023

jorgemanrubia left a comment •

edited

Loading

bradgessler commented Dec 19, 2023

bradgessler commented Dec 19, 2023

jorgemanrubia left a comment

dhh commented Dec 26, 2023

jorgemanrubia commented Dec 26, 2023 •

edited

Loading

dhh commented Dec 26, 2023

dhh commented Dec 27, 2023

Separate turbo meta tag generation from provides #542

Separate turbo meta tag generation from provides #542

Conversation

bradgessler commented Dec 13, 2023 • edited Loading

jorgemanrubia commented Dec 13, 2023

bradgessler commented Dec 13, 2023

jorgemanrubia commented Dec 13, 2023

bradgessler commented Dec 14, 2023

jorgemanrubia left a comment • edited Loading

Choose a reason for hiding this comment

bradgessler commented Dec 19, 2023

bradgessler commented Dec 19, 2023

jorgemanrubia left a comment

Choose a reason for hiding this comment

dhh commented Dec 26, 2023

jorgemanrubia commented Dec 26, 2023 • edited Loading

dhh commented Dec 26, 2023

dhh commented Dec 27, 2023

bradgessler commented Dec 13, 2023 •

edited

Loading

jorgemanrubia left a comment •

edited

Loading

jorgemanrubia commented Dec 26, 2023 •

edited

Loading