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

Base auto-capture logic on yield calls in template source #45

Merged
merged 2 commits into from
Jul 27, 2022

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Jul 6, 2022

Extracted from #44

Currently, we're basing our auto-capture logic on whether there's a
single argument passed to render, e.g.:

render "card" do |partial|
  partial.yield :title, t(".title")
end

But if users want to pass extra content via an explicit <%= yield something %>,
we don't support that.

This changes our logic to look for <%= yield something %> calls in the
template source and not auto-capture in that case. We will pass the partial
on as an extra argument in case callers want to expose that as a named block
argument.

We'll still auto-capture in case a template has non-capturing yields like
<%= yield :title %>.

Currently, we're basing our auto-capture logic on whether there's a
single argument passed to render, e.g.:

```ruby
render "card" do |partial|
  partial.yield :title, t(".title")
end
```

But if users want to pass extra content via an explicit `<%= yield something %>`,
we don't support that.

This changes our logic to look for `<%= yield something %>` calls in the
template source and not auto-capture in that case. We will pass the partial
on as an extra argument in case callers want to expose that as a named block
argument.

We'll still auto-capture in case a template has non-capturing yields like
`<%= yield :title %>`.
@kaspth kaspth force-pushed the base-auto-capture-on-template-source branch from a929d2f to 942d4c3 Compare July 8, 2022 21:32
@kaspth
Copy link
Contributor Author

kaspth commented Jul 8, 2022

Note: this also relies on some code structure in ActionView::PartialRenderer that was added in 6.1. I don't know how tied we are to backwards compatibility, but with the _layout_for override users would just need to insert a plain <%= yield %> into their partials, or we could look at explicit versions support for Rails 5.2 and possibly further down. What do you say @andrewculver?

@andrewculver
Copy link
Contributor

@kaspth No need to provide backward compatibility. Happy to support 6.1 and up.

@andrewculver andrewculver merged commit 1f71ec0 into main Jul 27, 2022
# Note: `<%= yield %>` becomes `yield :layout` with no `render` `block`, though this method assumes a block is passed.
def has_capturing_yield?
defined?(@has_capturing_yield) ? @has_capturing_yield :
@has_capturing_yield = source.match?(/\byield[\(? ]+(%>|[^:])/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticing the %>: does this work with HAML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I wanted to define this after template compilation, so we'd be Template Handler independent, but I just realized we'd need to trigger compilation for that to work (and that's why it didn't work).

I've never used anything besides ERB, how does HAML syntax go by the way? Like this?

%= yield
%= yield :title

Do you know what Slim looks like?

Copy link
Contributor

@domchristie domchristie Jul 27, 2022

Choose a reason for hiding this comment

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

I'm not a HAML/Slim user, but the %> got me thinking.

I've never used anything besides ERB, how does HAML syntax go by the way? Like this?

Yep I think so.

Slim looks similar http://slim-lang.com/

@kaspth kaspth deleted the base-auto-capture-on-template-source branch July 27, 2022 13:02
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.

None yet

3 participants